Skip to content
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

avoid running a clean-all before integration tests, which are invoked in a new buildroot #7522

Conversation

Projects
None yet
3 participants
@cosmicexplorer
Copy link
Contributor

commented Apr 9, 2019

Problem

Running the below command results in a clean-all being run before the integration test:

> ./pants2 test tests/python/pants_test/backend/python/tasks:: -- -s -k test_platform_defaults_to_config
...
10:32:53 00:06     [pytest]
                   Invalidated 21 targets.
                   scrubbed PYTHONPATH=/Users/dmcclanahan/tools/pants/src/python: from py.test environment
10:32:53 00:06       [run]
                     ============== test session starts ===============
                     platform darwin -- Python 2.7.10, pytest-3.6.4, py-1.7.0, pluggy-0.7.1
                     rootdir: /Users/dmcclanahan/tools/pants/.pants.d, inifile: /Users/dmcclanahan/tools/pants/.pants.d/test/pytest-prep/CPython-2.7.10/718d17b455ce5917535680e933c8a655f6334861/pytest.ini
                     plugins: timeout-1.2.1, cov-2.4.0
                     collected 142 items / 141 deselected

                     tests/python/pants_test/backend/python/tasks/test_python_binary_integration.py .python-interpreter-constraints not found in the buildroot.
                     Forcing an initial clean-all.
                     Different interpreter constraints were set than were used in the previous Pants run.
                     Clearing the cache and preparing a Python environment with these interpreter constraints:
                     ['CPython>=2.7,<3','CPython>=3.6,<4']
...

This is an artifact of the changes for #7151, which tries to be safe and clean-all if the .python-interpreter-constraints file doesn't exist to ensure subprocesses are invoked with the right python version. But we don't need to do this for integration tests, since they're invoked in a new buildroot.

Solution

  • Add ONLY_USING_SINGLE_PYTHON_VERSION=true to the env in all integration tests.

Result

Pants will no longer run a clean-all before integration tests in a new buildroot.

@cosmicexplorer cosmicexplorer requested review from blorente and Eric-Arellano Apr 9, 2019

@blorente

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

Is it worth it linking .pyton-interpreter-constraints in mock_buildroot()? I can see an argument against it if the file is expected to dissappear soon :)

@blorente
Copy link
Contributor

left a comment

Otherwise, lgtm

@Eric-Arellano
Copy link
Contributor

left a comment

Thanks! I’ll also be trying to fix the original issue via #7510. In the meantime this is a good fix.

@blorente

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

This was locally breaking any test that created a mock_buildroot, because it links the parent .pantsd.d to the temporary workdir of the pants under test, and therefore that link was removed when the clean-all ran.
Not sure we can easily add a test for this, but any ideas?

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

This was locally breaking any test that created a mock_buildroot

This was only happening locally because we happen to set the ONLY_USING_SINGLE_PYTHON_VERSION env var in our CI environment. Given that this fails pretty loudly, I think that we may not need a separate test for this change, since we have eliminated the difference between local and CI in this PR by forcing the env var to be set.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

I think that we may not need a separate test for this change.

I'm hesitant for us to spend much more engineering effort on this auto clean-all effort. These are all temporary workarounds (and very useful ones at that!) for the upstream issue. The better approach is to focus on fixing upstream via #7510.

@cosmicexplorer cosmicexplorer merged commit eebf02f into pantsbuild:master Apr 9, 2019

1 check passed

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

Eric-Arellano added a commit that referenced this pull request Apr 19, 2019

Rerun `select-interpreter` if global Python interpreter constraints h…
…ave changed (#7586)

### Problem
Currently we only check for changes to the file's `compatibility` entry in its `BUILD` file, but really we should be also checking global constraints.

As it stands now, changing `--python-setup-interpreter-constraints` will have no impact until running `./pants clean-all`, which is not meant to be.

This closes #7510.

### Solution
Use `PythonSetup`'s `compatibility_or_constraints` in the fingerprint strategy, which first checks if the targets have `compatibility` marked in their BUILD entries, and then falls back to the global interpreter constraints.

#### Alternative solution: invalidate upon subsystem changes
Alternatively, we could add `self.fingerprint` to our file naming scheme, so that _any_ invalidation to `PythonSetup` or to `PythonInterpreterCache` would also invalidate this task. See d35e80c#diff-3d23a0b5af958441c095d5acd551ac16R101 for this implementation.

We did not go down this route because it would result in more invalidation than necessary. This task is only ever impacted by changes to `--python-setup-interpreter-constraints` and `compatibility` entries.

#### Also revert auto clean-all
#7151 and #7522 were workarounds to this underlying issue.

### Result
Calling `PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="['CPython==3.6.*']" ./pants test --cache-ignore tests/python/pants_test/util:strutil` the first time will cause an invalidation, then the interpreter selection task will not run upon subsequent invocations.

Running `PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="['CPython==2.7.*']" ./pants test --cache-ignore tests/python/pants_test/util:strutil` right after will properly cause an invalidation and result in Python 2.7 being used for the tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.