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

Straighten out interpreter search path configuration #6849

Merged
merged 5 commits into from Dec 6, 2018

Conversation

Projects
None yet
3 participants
@benjyw
Copy link
Contributor

benjyw commented Nov 30, 2018

Previously, a value in a pexrc would completely override the value of the --interpreter-search-paths option. This change allows you to have any combination of fixed paths, a pexrc value, the env PATH value, and - as a new feature - interpreters managed by pyenv.

Note that this slightly changes the previous behavior, in that the default behavior (if no --interpreter-search-paths are specified) is now pexrc + PATH, whereas previously it was pexrc or PATH.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Dec 1, 2018

I don't think the slight behavior change mentioned above warrants a deprecation cycle, but let me know if you disagree.

@benjyw benjyw requested review from jsirois and CMLivingston Dec 1, 2018

'<PYENV>': lambda: cls.get_pyenv_paths(pyenv_root_func=pyenv_root_func)
}
expanded = []
for s in interpreter_search_paths:

This comment has been minimized.

@CMLivingston

CMLivingston Dec 1, 2018

Contributor

In accordance with the previous state of the code, PEXRC_PATH should take precedent no matter what. If it is set, it is the first thing that Pants searches for. This is intended to provide alignment with the underlying pex library's behavior when these vars are present in a .pexrc somewhere. I think it wouldn't be unreasonable to always prepend the PEXRC_PATH func results to the search paths if the return value is not empty. Thoughts?

This comment has been minimized.

@benjyw

benjyw Dec 2, 2018

Contributor

Keep in mind that the current behavior doesn't add the pexrc values to the option values - it overrides the option values entirely. And that's exactly what I'm trying to alter with this change. I don't think it makes sense to silently ignore a configured option because of the accidental presence of a pexrc file on a system. If a repo wants to always prepend the pexrc path, they can do this via the option, but there's no reason to force this.

Note that it's not a matter of precedence. The order of these paths is ignored - the interpreter cache looks for interpreters at every entry in the path, and chooses the lowest version that's compatible with all constraints.

This comment has been minimized.

@CMLivingston

CMLivingston Dec 3, 2018

Contributor

the interpreter cache looks for interpreters at every entry in the path, and chooses the lowest version that's compatible with all constraints.

This is a result of these changes afaict. Previously if pexrc is set, you are getting the lowest compatible interpreter within that setting, with no possibility of a system/other interpreter sliding underneath the pexrc ones at selection time.

How would you feel about defaulting to only the pexrc variable so the PATH interpreters cannot be selected as the min interpreter? Also, it would be great to add logging for the case where pants detects pexrc but ignores it in favor of custom paths (in the event that the user is unaware of Pants respecting it to ensure maximum alignment for build/runtime interpreters).

This comment has been minimized.

@benjyw

benjyw Dec 4, 2018

Contributor

If you want the previous behavior all you have to do is set the value of --interpreter-search-paths to [<PEXRC>]. The problem with having that as the default is that then Pants won't work out of the box for anyone who doesn't have a PEXRC set. This feels like something a repo needs to consciously choose. If I understand the pexrc feature correctly, it's for hermeticity? But the old behavior wasn't truly hermetic, in that if the user didn't have a pexrc, they'd silently fall back to their PATH. It feels like if a repo demands hermeticity then interpreter selection should fail without a pexrc, no?

This comment has been minimized.

@CMLivingston

CMLivingston Dec 4, 2018

Contributor

I see what you are saying. The older feature was to ensure hermeticity if pexrc is set and give that precedence over everything else. This was more of an implicit feature than something the user consciously chose to do, like "nice thing that Pants would make sure happens on your behalf."

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Dec 2, 2018

PS I renamed PEXRC_PATH to just PEXRC. The suffix adds nothing to comprehension.

@CMLivingston
Copy link
Contributor

CMLivingston left a comment

I added a comment about minimum interpreter selection. I think we should prevent system interpreters from being selected as min interpreters if they happen to have a lower version then a pexrc interpreter set.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Dec 4, 2018

I'll note that I've hung back on this one since @CMLivingston is more in touch with the point of a .pexrc.

@CMLivingston
Copy link
Contributor

CMLivingston left a comment

This LGTM from a functional perspective. The only thing I want to call out here is that implicit hermeticity will be broken for pexrc users with the default search path setting if $PATH contains an interpreter that matches target constraints but is an older version than anything set by a pexrc. I think it would be useful to have info level logging if a pexrc is in play and gets undercut by a PATH interpreter during min() interpreter selection, or at least log at a debug level that pexrc and path are being searched for a minimum matching interpreter and you may not be getting truly hermetic behavior. This is a nice-to-have so I'll leave that to you as to whether you want to add it. Thanks!

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Dec 5, 2018

Happy to add that logging.

benjyw added some commits Nov 30, 2018

@benjyw benjyw force-pushed the benjyw:interpreter_search_path_config branch from b00fc36 to 22d06e4 Dec 5, 2018

@benjyw benjyw merged commit 3275416 into pantsbuild:master Dec 6, 2018

1 check passed

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

@benjyw benjyw deleted the benjyw:interpreter_search_path_config branch Dec 6, 2018

@CMLivingston

This comment has been minimized.

Copy link
Contributor

CMLivingston commented Dec 11, 2018

FWIW, this broke our internal release candidate due to the path interpreters winning the min selection. Not a problem, but it did highlight the possibility that without knowing about interpreter_search_paths, users can be unsure of what to do. We might want to call out the exact option and setting in the logging to educate users on the behavior change given that there is no deprecation cycle. I can look into that.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Dec 11, 2018

Did you fix this by removing the entry from the option value?

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Dec 11, 2018

I think Twitter is atypical here. Most users, I'm guessing, want as many interpreters as possible to be found, and are relying on their PATH for this. The PEXRC thing is the advanced thing, and I'm not sure anyone outside of Twitter is using it? It's a great way to whitelist interpreters, but it's not Pants 101...

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