New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix local execution of hermetic integration tests #6101

Merged
merged 2 commits into from Jul 15, 2018

Conversation

Projects
None yet
3 participants
@stuhood
Copy link
Member

stuhood commented Jul 12, 2018

Problem

Post #5742, def hermetic(cls): True integration tests fail in environments where PATH or PY are not set due to a non-zero exit code from which python2.7, which causes a failure of the script due to set -e.

Solution

Exit the subshell cleanly if python2.7 cannot be located.

Result

Hermetic integration tests work locally and/or on OSX.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 12, 2018

I'd love suggestions for how to test this in travis. I am more than a little bit sketched out, because I'm not sure why this is not failing there.

But to repro the failure, try running

./pants test tests/python/pants_test/backend/jvm/tasks:scalafix -- -s -v -k test_scalafix_disabled

...on master. Add set -ex to the pants script to see it stop abruptly at the which line.

pants Outdated
@@ -47,7 +47,7 @@ source ${HERE}/build-support/bin/native/bootstrap_code.sh

PANTS_EXE="${HERE}/src/python/pants/bin/pants_loader.py"

PY=${PY:-$(which python2.7)}
PY=${PY:-$(which python2.7 || exit 0)}

This comment has been minimized.

@stuhood

stuhood Jul 12, 2018

Member

It is also not obvious that anything uses this variable, as it seems like all consumers calculate it directly: ie, https://github.com/pantsbuild/pants/blob/master/build-support/virtualenv#L16

This comment has been minimized.

@stuhood

stuhood Jul 12, 2018

Member

...kicked off another travis run to try this out over here, as it seems to work locally: #6102

This comment has been minimized.

@illicitonion

illicitonion Jul 12, 2018

Contributor

Would be good to remove this if possible; sadly your CI run was polluted by the bad wheel thing :(

This comment has been minimized.

@jsirois

jsirois Jul 12, 2018

Member

IIRC this was here to support Twitter internal scripts at some distant point in time. I'd definitely kill.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 12, 2018

I'd love suggestions for how to test this in travis. I am more than a little bit sketched out, because I'm not sure why this is not failing there.

Aha. It's likely because we run pants as a pex there.

@illicitonion
Copy link
Contributor

illicitonion left a comment

Hurrah!

@@ -7,7 +7,7 @@ set -e
ROOT=$(cd $(dirname "${BASH_SOURCE[0]}") && cd "$(git rev-parse --show-toplevel)" && pwd)
source ${ROOT}/build-support/common.sh

PY=$(which python2.7)
PY=$(which python2.7 || exit 0)

This comment has been minimized.

@illicitonion

illicitonion Jul 12, 2018

Contributor

I'd maybe write this as:

PY=$(which python2.7 || die "You must have python2.7...")
export PY

because exiting 0 when what you want to do is fail seems a little weird :)

This comment has been minimized.

@stuhood

stuhood Jul 12, 2018

Member

AFAIK, this exits the subshell, not the whole script. This is the actual fix for the issue here... the changes in pants_run_integration are just noise.

EDIT: Whoops. Sorry. Wrong file. Yes, will update.

This comment has been minimized.

@stuhood

stuhood Jul 13, 2018

Member

Er, same thing applies there I think. We'll die on the next line with the message.

pants Outdated
@@ -47,7 +47,7 @@ source ${HERE}/build-support/bin/native/bootstrap_code.sh

PANTS_EXE="${HERE}/src/python/pants/bin/pants_loader.py"

PY=${PY:-$(which python2.7)}
PY=${PY:-$(which python2.7 || exit 0)}

This comment has been minimized.

@illicitonion

illicitonion Jul 12, 2018

Contributor

Would be good to remove this if possible; sadly your CI run was polluted by the bad wheel thing :(

@jsirois
Copy link
Member

jsirois left a comment

This seems like whac-a-mole, although likely a pretty stable set of moles in the pants script. From a high level, invoking the ./pants shell script should legitmately be expected to utilize PATH to get its job done (the motivation for #6017). It certainly solves things for now though.

pants Outdated
@@ -47,7 +47,7 @@ source ${HERE}/build-support/bin/native/bootstrap_code.sh

PANTS_EXE="${HERE}/src/python/pants/bin/pants_loader.py"

PY=${PY:-$(which python2.7)}
PY=${PY:-$(which python2.7 || exit 0)}

This comment has been minimized.

@jsirois

jsirois Jul 12, 2018

Member

IIRC this was here to support Twitter internal scripts at some distant point in time. I'd definitely kill.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 12, 2018

Oh jeez. Totally forgot about #6017... sorry. I did track this down to "succeeding if PATH is present, failing otherwise", then added set -ex and noticed where it was failing.

The open question, then, is how do we end up succeeding without a PATH? The motivation for #6017 is that it isn't possible to succeed without a path, but we seem to here.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Jul 12, 2018

The open question, then, is how do we end up succeeding without a PATH?

After pants has been run once to bootstrap (building ./pants.pex in CI) it just happens to be the case that only bash builtin functions are used. My argument is that this just-so-happens is ~lucky and not to be expected by any shell script in general. The ameliorating factor is that we own the pants script and so the field of moles to whack is under our control so this seems acceptable but not ideal.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Jul 12, 2018

Hmm, except #!/usr/bin/env bash at the top of the script. Somehow that still works but I'm not sure how. Would need to dig into env which maybe uses /proc/self/environ on linux?

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 12, 2018

And more generally, hermetic integration tests have never had a PATH, and have always succeeded. They only began failing with the set -e (based on my bisect).

stuhood added some commits Jul 12, 2018

Fix local execution of hermetic integration tests, in which `which py…
…thon2.7` was failing due to PATH being cleared.
Completely remove the PY variable in the pants script, as the virtual…
…env code already seems to do the right thing.

@stuhood stuhood force-pushed the twitter:stuhood/fix-local-execution-of-hermetic-integration-tests branch from 49f387a to 96cece0 Jul 13, 2018

@stuhood stuhood merged commit 17d6b83 into pantsbuild:master Jul 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:stuhood/fix-local-execution-of-hermetic-integration-tests branch Jul 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment