rpms / clang

Clone
Source Code
GIT

#133 tests: parametrize "libc++" in tests
Merged 2 years ago by sergesanspaille. Opened 2 years ago by happz.
rpms/ happz/clang tests-parametrize-libc++  into  rawhide

file modified
+27 -9 
@@ -1,12 +1,30 @@ 
 

  # TODO REVIEW: better summary
 

  summary: ""
 

  test: ./test.sh
 

- require:
 

-   - clang
 

-   - lld
 

-   - compiler-rt
 

-   - libcxx-devel
 

-   - libcxx-static
 

-   - glibc-devel
 

-   - glibc-static
 

-   - gcc
 

+ adjust:
 

+   - environment+:
 

+       CXXLIB: "libc++"
 

+     require:
 

+       - clang
 

+       - lld
 

+       - compiler-rt
 

+       - libcxx-devel
 

+       - libcxx-static
 

+       - glibc-devel
 

+       - glibc-static
 

+       - gcc
 

+     when: "distro == fedora"
 

+     because: testing against libcxx package in Fedora
 

+   - environment+:
 

+       CXXLIB: "libstdc++"
 

+     require:
 

+       - clang
 

+       - lld
 

+       - compiler-rt
 

+       - glibc-devel
 

+       - glibc-static
 

+       - gcc
 

+       - libstdc++
 

+     when: "distro == rhel"
 

+     because: testing against libstdc++ package in RHEL as libcxx is not shipped with RHEL
 

+ require: []

file modified
+7 -1 
@@ -4,6 +4,12 @@ 
 

  

  set pipefail
 

  

+ if [ -z "${CXXLIB:-}" ]; then
 

+   echo "CXXLIB variable is a required input but it's not specified!"
 

+   echo "Test metadata should have picked a proper value, depending on distro."
 

+   exit 1
 

+ fi
 

+ 

  # Test compile a C program.
 

  cat << EOF | \
 

  	clang -fuse-ld=lld -rtlib=compiler-rt -x c - && \
 
@@ -18,7 +24,7 @@ 
 

  

  # Test compile a C++ program.
 

  cat << EOF | \
 

- 	clang++ -x c++ -fuse-ld=lld -rtlib=compiler-rt -stdlib=libc++ - && \
 

+ 	clang++ -x c++ -fuse-ld=lld -rtlib=compiler-rt -stdlib="$CXXLIB" - && \
 

  	./a.out | grep 'Hello World'
 

  

  #include <iostream>

file modified
+16 -2 
@@ -1,7 +1,21 @@ 
 

  # TODO REVIEW: better summary
 

  summary: ""
 

  test: ./test.sh
 

+ adjust:
 

+   - environment+:
 

+       CXXLIBS: "libc++"
 

+     require:
 

+       - clang
 

+       - libcxx-devel
 

+     when: "distro == fedora"
 

+     because: testing against libcxx package in Fedora
 

+   - environment+:
 

+       CXXLIBS: "libstdc++"
 

+     require:
 

+       - clang
 

+       - libstdc++
 

+     when: "distro == rhel"
 

+     because: testing against libstdc++ package in RHEL as libcxx is not shipped with RHEL
 

  # TODO REVIEW: are these all requirements? test.sh seems to run quite a lot of stuff, looks like we
 

  # need more packages from LLVM family.
 

- require:
 

-   - clang
 

+ require: []

file modified
+10 -1 
@@ -2,6 +2,12 @@ 
 

  

  set pipefail
 

  

+ if [ -z "${CXXLIBS:-}" ]; then
 

+   echo "CXXLIBS variable is a required input but it's not specified!"
 

+   echo "Test metadata should have picked a proper value, depending on distro."
 

+   exit 1
 

+ fi
 

+ 

  status=0
 

  

  test_toolchain() {
 
@@ -25,6 +31,9 @@ 
 

              libc++)
 

                  args="$args -stdlib=$1"
 

                  ;;
 

+             libstdc++)
 

+                 args="$args -stdlib=$1"
 

+                 ;;
 

              lld)
 

                  args="$args -fuse-ld=$1"
 

                  ;;
 
@@ -54,7 +63,7 @@ 
 

  for compiler in clang clang++; do
 

      for rtlib in "" compiler-rt; do
 

          for linker in "" lld; do
 

-             for cxxlib in "" libc++; do
 

+             for cxxlib in "" $CXXLIBS; do
 

                  if [ "$compiler" = "clang" -a -n "$cxxlib" ]; then
 

                      continue
 

                  fi

Downstream has no libc++, but it can run clang tests with libstdc++.
Parametrize the tests, leaving the actual library to use on their
respective main.fmf files that are easier to overturn in downstream
repos.

LGTM

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

[citest]

I wonder if it's better to have a default value or to warn if no value are given? That would limit the chance of not testing the expected library.

I wonder if it's better to have a default value or to warn if no value are given? That would limit the chance of not testing the expected library.

I agree. I prefer tests that are not allowed to guess, and the default value is sort of guessing what to test. I'd drop the default value completely, & fail the test if it's left undefined.

I also switched the PR to WiP mode, because I'd like to try a slightly different approach, something like this:

adjust:
    - environment+:
          CXXLIB: "libc++"
      when: "distro ~= "fedora-.+"
    - environment+:
          CXXLIB: "libstdc++"
      when: "distro ~= "rhel-.+"

It should reduce the amount of stuff needed to tweak downstream to zero, while not making things too complicated for "upstream" Fedora folks, and all test metadata would be at one place, rather than RHEL having its own special copy.

I suppose that tests landing in Fedora could live without any adjust, until we realize downstream needs something slightly different.

But I've learned this should be possible today, so no experience with the behavior.

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

I wonder if it's better to have a default value or to warn if no value are given? That would limit the chance of not testing the expected library.

Done, CXXLIB and CXXLIBS now do have a "default" which is an empty string, to avoid "unbound variable" error thrown by shell and rather provide a more helpful error message in the log.

And variable itself now depends on TMT's context which is something CI systems set, and testing distro seems to work like a charm (tested with both Fedora and RHEL). So, no need to modify this test when synchronized into downstream repositories.

Please, review & I'd be looking forward for positive outcome.

rebased onto c030682c2dd428e166f5904b27690638f9a6f162
2 years ago

rebased onto 89b80b93eb9eb0f330246d0092a3b0644e61cce5
2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto 60dba4e1dda3ddc4f33a948b9708af2e94cd10b2
2 years ago

rebased onto 19d960a8183183a3759732109df7a713160eb269
2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

Everything's done, ready for a review & hopefully a merge.

@happz : the rpm-tmt-test failure seems blocking, right?

@happz : the rpm-tmt-test failure seems blocking, right?

@sergesanspaille Hm, that's a good question. I did not bother to inspect Zuul's results, because, well, the dist-git test is green and I touched nothing else. Anyway, after some digging, this is a bug in Zuul, not in tests, and it's now reported in https://pagure.io/fedora-ci/general/issue/298 (Fedora CI sets correct context for TMT, context: {'trigger': ['commit'], 'arch': ['x86_64'], 'distro': ['fedora-36']}, Zuul does not: context: {}).

While I'd appreciate having this merged since it's a blocker for my work downstream, it's still your repository, and I don't know much about Zuul's rulings in your Fedora workflows. Feel free to call it a blocker, I can only hope Zuul's fix would land soon.

@happz : the Zuul seems fixed upstream, I'm not sure it's in production, let's test that

recheck

[citest]

[citest]

[citest]

rebased onto 5f6796d
2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

I'm going to merge that one. The errors are unrelated and I understand the burden for Milos.

I have the same issue with s390x builder on other PRs :-/

Pull-Request has been merged by sergesanspaille
2 years ago
Metadata
None
No Tags
Changes Summary 4
+27 -9
file changed
tests/llvm-toolchain/main.fmf
+7 -1
file changed
tests/llvm-toolchain/test.sh
+16 -2
file changed
tests/toolchains/main.fmf
+10 -1
file changed
tests/toolchains/test.sh
Powered by Pagure 5.13.3
Documentation • File an Issue • About this Instance • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.