This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by tstellar on Feb 10 2017, 11:13 AM.

Event Timeline

tstellar created this revision.Feb 10 2017, 11:13 AM

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.

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.

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

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

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 updated this revision to Diff 89874.Feb 27 2017, 6:45 AM

Build and run the test suite when do_test_release=yes

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 added inline comments.Feb 27 2017, 9:04 AM
utils/release/test-release.sh
413

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.

415

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

tstellar updated this revision to Diff 90081.Feb 28 2017, 1:55 PM
  • Clean up code using variables and make it clear we want to return to BuildDir
  • Make sure test output is logged.
rengolin accepted this revision.Mar 1 2017, 2:48 AM

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.

This revision is now accepted and ready to land.Mar 1 2017, 2:48 AM
This revision was automatically updated to reflect the committed changes.
rovka added a subscriber: rovka.Jul 27 2017, 6:41 AM

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

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.