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

Bugfix for stale interpreter.info purging when binary exe is no longer on the filesystem #7292

Conversation

Projects
None yet
4 participants
@CMLivingston
Copy link
Contributor

commented Feb 27, 2019

The stale interpreter purge logic was not handling interpreter.info file correctly due to inaccurate mocking during manual testing, resulting in a ExecutableNotFound error when it encounters a interpreter.info file written by Pants.

This change wraps calls to _get_interpreter with a try/except so we do not try to load a PythonInterpreter object from a binary that does not exist on the filesystem.

To test this, I used the source workflow in our monorepo, and removed the cached Python 3.7 binary executable itself from the filesystem, resulting in the following output:

$ ./pants run python/tests/interpreter_verification:python3_main

14:08:13 00:00 [main]
               (To run a reporting server: ./pants server)
14:08:15 00:02   [setup]
14:08:15 00:02     [parse]
...
14:08:17 00:04   [native-compile]
14:08:17 00:04     [conan-prep]INFO] Detected stale interpreter `/Users/clivingston/workspace/source3/.pants.d/python-setup/interpreters/CPython-2.7.15` in the interpreter cache, purging.
INFO] Detected stale interpreter `/Users/clivingston/workspace/source3/.pants.d/python-setup/interpreters/CPython-3.7.2` in the interpreter cache, purging.

14:08:22 00:09     [conan-fetch]
...
14:08:22 00:09   [pyprep]
14:08:22 00:09     [interpreter]
                   Stale interpreter reference detected: /Users/clivingston/workspace/source3/.pants.d/pyprep/interpreter/a31e7e1381463c9d3652d70ba8f9f7b6345ddf8b/interpreter.info, removing reference and selecting a new interpreter.
...
14:08:26 00:13   [run]
14:08:26 00:13     [py]
14:08:27 00:14       [run]
Python 3 in Source!

@CMLivingston CMLivingston requested a review from stuhood Feb 27, 2019

@stuhood
Copy link
Member

left a comment

It would be good to add a test I think, since it sounds like the testing was insufficient to detect this the first time.

Maybe something like: "making a copy of the interpreter cache, and then corrupt it so that an entry can't be used"?

@CMLivingston

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

A test already exists for that code path that was included in the first iteration - this is the select_interpreter side of things that deals with interpreter.info metadata.

@CMLivingston

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

This has been verified internally to Twitter via source workflow.

@Eric-Arellano
Copy link
Contributor

left a comment

Fantastic!

I think this means we can simplify pants_venv as follows:

diff --git a/build-support/pants_venv b/build-support/pants_venv
index 457d48098..9114d887c 100755
--- a/build-support/pants_venv
+++ b/build-support/pants_venv
@@ -24,18 +24,8 @@ function activate_venv() {

 function remove_venv() {
   rm -rf "$(venv_dir)"
-  # If `pants_dev_deps.venv` or `pants_dev_deps.py{2,3}.venv` still exist, delete both the legacy folders
-  # and the cached Python interpreter folder, which contains problematic symlinks. Note we only perform
-  # these removals for the first time, to avoid continually deleting the cache when switching
-  # between valid venvs.
-  legacy_venv_dirs=("${venv_dir_prefix}.venv" "${venv_dir_prefix}.py2.venv" "${venv_dir_prefix}.py3.venv")
-  for legacy_venv_dir in "${legacy_venv_dirs[@]}"; do
-    if [ -d "${legacy_venv_dir}" ]; then
-      rm -rf "${legacy_venv_dirs[@]}"
-      rm -rf "${HOME}/.cache/pants/python_cache/interpreters"
-      break
-    fi
-  done
+  # Also remove legacy folders.
+  rm -rf "${venv_dir_prefix}.venv" "${venv_dir_prefix}.py{2,3}.venv"
 }

 function create_venv() {

I'm in class next hour so can't verify. But if this sounds right, please feel free to add this change.

@stuhood
Copy link
Member

left a comment

Thanks!

Chris Livingston
@baroquebobcat
Copy link
Contributor

left a comment

Looks reasonable to me. I have one test assert suggestion though.

workdir=workdir,
config=config
)
self.assert_success(pants_run)

This comment has been minimized.

Copy link
@baroquebobcat

baroquebobcat Feb 28, 2019

Contributor

Rather than just success, this could assert that the interpreter.infos no longer contain the bogus paths, or that the debug log was printed.

I think that might make it more clear what the expected outcome is.

This comment has been minimized.

Copy link
@CMLivingston

CMLivingston Feb 28, 2019

Author Contributor

Agreed, I'll add the former.

This comment has been minimized.

Copy link
@baroquebobcat

baroquebobcat Feb 28, 2019

Contributor

👍

Chris Livingston added some commits Feb 28, 2019

Chris Livingston
Chris Livingston

@CMLivingston CMLivingston merged commit ee101f3 into pantsbuild:master Mar 6, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.