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

Make interpreter path expansion cache-correct #17051

Merged

Conversation

chrisjrn
Copy link
Contributor

@chrisjrn chrisjrn commented Sep 28, 2022

Following discussion with @stuhood, it emerged that none of the filesystem-sensitive code in python_bootstrap is cache-correct, in that the functions that hit the filesystem were not marked uncacheable.

This PR refactors _expand_interpreter_search_paths and _get_pyenv_paths into rules, marking _get_pyenv_paths as uncacheable. This reduces the footprint of the sandbox-unsafe code to just _get_pyenv_paths.

It also reduces the scope of AsdfToolPathsRequest, and moves the point where it is requested to inside the path expansion rule, and only if asdf is actually requested.

Closes #16800.

@chrisjrn chrisjrn marked this pull request as ready for review September 28, 2022 22:46
@chrisjrn chrisjrn changed the title Make interpreter patch expansion cache-correct Make interpreter path expansion cache-correct Sep 28, 2022
@chrisjrn chrisjrn added the category:internal CI, fixes for not-yet-released features, etc. label Sep 28, 2022
@chrisjrn chrisjrn marked this pull request as draft September 29, 2022 15:28
@chrisjrn
Copy link
Contributor Author

(Moved back to draft status while I address ASDF in the same PR. Apparently asdf.py is only used in Python bootstrapping at this stage.

@chrisjrn chrisjrn marked this pull request as ready for review September 29, 2022 16:10
@Eric-Arellano
Copy link
Contributor

This is probably "bugfix" - it impacts users for existing features

@chrisjrn
Copy link
Contributor Author

Eugh, looks like I do need to make the go support work as well. WHEEEEEE.

@chrisjrn chrisjrn marked this pull request as draft September 29, 2022 17:38
@chrisjrn
Copy link
Contributor Author

@Eric-Arellano Agreed, I'll change that over momentarily

Christopher Neugebauer added 6 commits September 29, 2022 15:47
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
… from elsewhere

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@chrisjrn chrisjrn added category:bugfix Bug fixes for released features and removed category:internal CI, fixes for not-yet-released features, etc. labels Sep 29, 2022
@chrisjrn chrisjrn force-pushed the chrisjrn/make-expansion-cache-efficient branch from 8f365d7 to 7d52aa6 Compare September 29, 2022 22:54
@chrisjrn chrisjrn marked this pull request as ready for review September 29, 2022 22:54
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

raise ValueError(
softwrap(
f"`[python-bootstrap].search_paths` is configured to use local Go discovery "
f"tools, which do not work in {env_type.__name__} runtime environments. To fix "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably instead be env_target.alias

@chrisjrn chrisjrn enabled auto-merge (squash) September 30, 2022 19:08
@chrisjrn chrisjrn merged commit dc5ca7b into pantsbuild:main Sep 30, 2022
@chrisjrn chrisjrn deleted the chrisjrn/make-expansion-cache-efficient branch September 30, 2022 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix filesystem sandbox leaks in Python bootstrapping
2 participants