test-release.sh: Remove workaround for test-suite build

[tstellar]

Summary

We aren't actually building the test suite, so this isn't needed.

[rengolin]

Well, this checks out the test-suite, which one can link to their LNT directory. I have been bitten before for running dot-release testing on trunk test-suite and spending a while realising it had new tests that weren't present in the original release.

Since we have a flag to disable that, I'm not sure there's value in simply removing it and forcing people to check it out on their own.

[tstellar]

In D29840#673825, @rengolin wrote:

Well, this checks out the test-suite, which one can link to their LNT directory. I have been bitten before for running dot-release testing on trunk test-suite and spending a while realising it had new tests that weren't present in the original release.

Since we have a flag to disable that, I'm not sure there's value in simply removing it and forcing people to check it out on their own.

The only change with this patch is that by default the test-suite will be checked out to ${tag}/llvm.src/projects rather than ${tag}/, so the test-suite will still be checked out. Would it be better to keep ${tag}/ as the default location ? I wasn't sure if the test-suite location mattered given that the test-release.sh script doesn't do anything with it.

[rengolin]

In D29840#673847, @tstellar wrote:

The only change with this patch is that by default the test-suite will be checked out to ${tag}/llvm.src/projects rather than ${tag}/, so the test-suite will still be checked out.

Right, I see. This will cause the test to be built by default as well, and not ran (CMake can't do that yet), so it'll only slow down the release for no reason.

Would it be better to keep ${tag}/ as the default location ?

Only if do_test_suite=export-only.

I'd say make the default be export-only in addition to your current patch, so that it gets built if the user requests it (do_test_suite=yes), but by default only be checked out, for external LNT validation.

I wasn't sure if the test-suite location mattered given that the test-release.sh script doesn't do anything with it.

AFAIK, CMake will build whatever is in the right place. Regardless, having it in ${tag} is easier for LNT integration, so I'd still keep export-only as the default under ${tag} and allow yes to move it to projects.

Makes sense?

--renato

[tstellar]

In D29840#681106, @rengolin wrote:

In D29840#673847, @tstellar wrote:

The only change with this patch is that by default the test-suite will be checked out to ${tag}/llvm.src/projects rather than ${tag}/, so the test-suite will still be checked out.

Right, I see. This will cause the test to be built by default as well, and not ran (CMake can't do that yet), so it'll only slow down the release for no reason.

It won't do this, because the cmake file in the project dir explicitly doesn't build the test-suite: https://github.com/llvm-mirror/llvm/blob/master/projects/CMakeLists.txt.

Would it be better to keep ${tag}/ as the default location ?

Only if do_test_suite=export-only.

I'd say make the default be export-only in addition to your current patch, so that it gets built if the user requests it (do_test_suite=yes), but by default only be checked out, for external LNT validation.

I wasn't sure if the test-suite location mattered given that the test-release.sh script doesn't do anything with it.

AFAIK, CMake will build whatever is in the right place. Regardless, having it in ${tag} is easier for LNT integration, so I'd still keep export-only as the default under ${tag} and allow yes to move it to projects.

Makes sense?

Yes, but I think I'll also update the patch so that do_test_suite=yes actually works.

--renato

[rengolin]

In D29840#686140, @tstellar wrote:

It won't do this, because the cmake file in the project dir explicitly doesn't build the test-suite: https://github.com/llvm-mirror/llvm/blob/master/projects/CMakeLists.txt.

Ah, excellent!

If that's the case, then keeping it in projects is harmless and probably simpler.

cheers,
--renato

[tstellar]

Build and run the test suite when do_test_release=yes

[tstellar]

In D29840#686154, @rengolin wrote:

In D29840#686140, @tstellar wrote:

It won't do this, because the cmake file in the project dir explicitly doesn't build the test-suite: https://github.com/llvm-mirror/llvm/blob/master/projects/CMakeLists.txt.

Ah, excellent!

If that's the case, then keeping it in projects is harmless and probably simpler.

cheers,
--renato

I ended up putting it in $proj.src, for consistency with the export-only case.

[rengolin]

utils/release/test-release.sh

409 ↗

(On Diff #89874)

I'd move this line to the end, and move the "cd $BuildDir" just after the if, to make it clear that we want to be in that dir as we leave the function. The behaviour is the same, but adding new stuff to the end may confuse people and be wrong.

411 ↗

(On Diff #89874)

This could be "$BuildDir/sandbox" and with double quotes, to make sure directories with spaces won't break.

[tstellar]

• Clean up code using variables and make it clear we want to return to BuildDir
• Make sure test output is logged.

[rengolin]

Right, there are many "offences" surrounding Bash quoting, but none of them are new, so the script will blow up much earlier if any path has spaces in it, so this is not a fix for this patch.

For that reason, LGTM. Thanks!

PS: We should pass ShellCheck on that script and do a single commit to fix all potential bashisnms, later.

[rovka]

Hi Tom,

do_test_suite is 'yes' by default, so now our release jobs are also trying to setup the test-suite. This isn't a problem in itself, but if the test-suite setup fails for whatever reason, we don't package the release anymore, which means we don't get a nice archive in the end. Would it be difficult to fix that, so we can get the binaries regardless of what happened with the test-suite?

Thanks,
Diana

[tstellar]

In D29840#822678, @rovka wrote:

Hi Tom,

do_test_suite is 'yes' by default, so now our release jobs are also trying to setup the test-suite. This isn't a problem in itself, but if the test-suite setup fails for whatever reason, we don't package the release anymore, which means we don't get a nice archive in the end. Would it be difficult to fix that, so we can get the binaries regardless of what happened with the test-suite?

Yes, that makes sense to me, I can fix that.

llvm/trunk/utils/release/test-release.sh

case $1 in
             echo "unknown option: $1"
             usage
             exit 1
             ;;
     esac
     shift
 done
 
-if [ "$do_test_suite" = "yes" ]; then
-  # See llvm.org/PR26146.
-  echo Skipping test-suite build when using CMake.
-  echo It will still be exported.
-  do_test_suite="export-only"
-fi
-
 # Check required arguments.
 if [ -z "$Release" ]; then
     echo "error: no release number specified"
     exit 1
 fi
 if [ -z "$RC" ]; then
     echo "error: no release candidate number specified"
     exit 1
for proj in $projects ; do
             ;;
         clang-tools-extra)
             projsrc=llvm.src/tools/clang/tools/extra
             ;;
         compiler-rt|libcxx|libcxxabi|libunwind|openmp)
             projsrc=llvm.src/projects/$proj
             ;;
         test-suite)
-            if [ $do_test_suite = 'yes' ]; then
-              projsrc=llvm.src/projects/$proj
-            else
-              projsrc=$proj.src
-            fi
             ;;
         *)
             echo "error: unknown project $proj"
             exit 1
             ;;
         esac
 
         if [ -d $projsrc ]; then
function test_llvmCore() {
     ObjDir="$3"
 
     cd $ObjDir
     if ! ( ${MAKE} -j $NumJobs -k check-all \
         2>&1 | tee $LogDir/llvm.check-Phase$Phase-$Flavor.log ) ; then
       deferred_error $Phase $Flavor "check-all failed"
     fi
 
+    if [ $do_test_suite = 'yes' ]; then
+      SandboxDir="$BuildDir/sandbox"
+      Lit=$SandboxDir/bin/lit
+      TestSuiteBuildDir="$BuildDir/test-suite-build"
+      TestSuiteSrcDir="$BuildDir/test-suite.src"
+
+      virtualenv $SandboxDir
+      $SandboxDir/bin/python $BuildDir/llvm.src/utils/lit/setup.py install
+      mkdir -p $TestSuiteBuildDir
+      cd $TestSuiteBuildDir
+      cmake $TestSuiteSrcDir -DTEST_SUITE_LIT=$Lit
+      if ! ( ${MAKE} -j $NumJobs -k check \
+          2>&1 | tee $LogDir/llvm.check-Phase$Phase-$Flavor.log ) ; then
+        deferred_error $Phase $Flavor "test suite failed"
+      fi
+    fi
     cd $BuildDir
 }
 
 # Clean RPATH. Libtool adds the build directory to the search path, which is
 # not necessary --- and even harmful --- for the binary packages we release.
 function clean_RPATH() {
   if [ "$System" = "Darwin" ]; then
     return