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

Fix pip-sync --python-executable evaluating markers for the wrong environment #2116

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gschaffner
Copy link
Member

@gschaffner gschaffner commented Jul 24, 2024

Proposed patchset that fixes #2115.

I've marked this as a draft PR because it's just a fix; it does not yet include an automated test. The test for this seems difficult to fit into the current testing framework—the test requires access to multiple (differing) interpreters—so I was hoping someone with more familiarity could help with that.

Contributor checklist
  • Included tests for the changes.
  • PR title is short, clear, and ready to be included in the user-facing changelog.
Maintainer checklist
  • Verified one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@gschaffner gschaffner added bug Something is not working markers Related to environment markers sync labels Jul 24, 2024
@gschaffner
Copy link
Member Author

I do not currently understand the failures of test_python_executable_option (assert 0 == 2) on various piplatest on GHA. tox -r -e "$(echo py{38,39,310,311,312,py3}-pip{previous,latest,main}-coverage | tr ' ' ',')" -- --no-cov -n=0 -v -k test_python_executable_option passes locally.

piptools/sync.py Outdated Show resolved Hide resolved
piptools/sync.py Outdated Show resolved Hide resolved
piptools/scripts/sync.py Outdated Show resolved Hide resolved
@chrysle chrysle added this to the 7.4.2 milestone Jul 26, 2024
@chrysle
Copy link
Contributor

chrysle commented Jul 26, 2024

The test for this seems difficult to fit into the current testing framework—the test requires access to multiple (differing) interpreters—so I was hoping someone with more familiarity could help with that.

Basically, we might just as well use the same interpreter and a corresponding marker?

@gschaffner
Copy link
Member Author

Basically, we might just as well use the same interpreter and a corresponding marker?

I don't think I understand, where are you suggesting putting a marker?

My understanding is that for the bug to occur, InstallRequirement.markers.evaluate(environment) needs to produce a different result with the pip-sync interpreter's environment and with the --python-executable=... interpreter's environment. AFAIK this is only possible if the two environments differ, and AFAIK that is only possible if they are not the same interpreter.

@gschaffner gschaffner force-pushed the fix-sync-external-python-marker-eval branch from b64111a to 64ebf70 Compare August 4, 2024 08:09
@gschaffner
Copy link
Member Author

Applied review suggestions and fixed a crash on Windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working markers Related to environment markers sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip-sync --python-executable evaluates markers for the wrong environment
2 participants