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 pyenv backend respect patch version constraints #21139

Merged

Conversation

jonasrauber
Copy link
Contributor

fixes #20175

fix inspired by #19462

@tdyas tdyas added category:bugfix Bug fixes for released features backend: Python Python backend-related issues labels Jul 8, 2024
This was referenced Jul 9, 2024
@huonw
Copy link
Contributor

huonw commented Jul 9, 2024

Thank you for contributing!

The code looks good to me, but I'll loop @thejcannon in too.

Could we add a test to src/python/pants/backend/python/providers/pyenv/rules_integration_test.py that validates that this works (and will continue to in future)?

One option might be evolving the test_using_pyenv test to take two parameters like ("interpreter_constraints", "expected_version_substring"), and then pass ("2.7.*", "2.7"), ("3.9.*", "3.9"), ("3.10.4", "3.10.4") or something like that. What do you think?

@pytest.mark.parametrize("py_version", ["2.7", "3.9"])
def test_using_pyenv(rule_runner, py_version):

@huonw huonw requested a review from thejcannon July 9, 2024 23:40
@jonasrauber
Copy link
Contributor Author

@huonw Added the test. Pretty much as you described. Also made sure that the test fails without this PR:

E       AssertionError: assert '3.10.4' in '3.10.10 (main, Jul  9 2024, 18:05:03) [Clang 15.0.0 (clang-1500.3.9.4)]'

With the fixes from this PR it passes.

Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Nice one! I like it.

Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Ah, I did another skim and thought of a few other things. Sorry about the dribble of reviews!

for (major, minor, patch) in supported_triplets
if major == major_to_use and minor == minor_to_use and patch <= latest_known_patch
)
except ValueError as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, on second-review: it'd be good to have a test that validates this logic too, i.e. interpreter constraints that constraint to a patch version that Pyenv doesn't know about (and hopefully will never know about, to avoid spurious failures in future).

src/python/pants/backend/python/providers/pyenv/rules.py Outdated Show resolved Hide resolved
src/python/pants/backend/python/providers/pyenv/rules.py Outdated Show resolved Hide resolved
@huonw
Copy link
Contributor

huonw commented Jul 26, 2024

Thanks for the contribution @jonasrauber! I've been nitpicky, so I'll fix those minor issues myself rather than force it onto you.

@huonw huonw merged commit 1a65ae2 into pantsbuild:main Jul 26, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: Python Python backend-related issues category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pyenv backend does not respect patch version constraints
3 participants