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 inconsistent handling of constraints comments with backtracking resolver #1713

Merged
merged 4 commits into from Nov 5, 2022
Merged

Fix inconsistent handling of constraints comments with backtracking resolver #1713

merged 4 commits into from Nov 5, 2022

Conversation

mkniewallner
Copy link
Contributor

@mkniewallner mkniewallner commented Oct 31, 2022

Resolves #1712.

Investigating the root cause of the issue led to what could be a faulty condition in the backtracking resolver.

self.existing_constraints will always be empty when a generated .txt file doesn't exist yet, but will contain all the generated file dependencies when it does exist, so the condition that is removed by this PR will always evaluate to False for all dependencies in this case.
Consequently, this condition in the writer will also evaluate to False, preventing the retrieval of the constraints annotations when an already existing .txt file is updated.

There may be a specific reason this condition is here, but since the information only seems relevant for annotating the dependencies, it doesn't seem to me that it is relevant.

The PR also adds a specific test that ensures that running pip-compile twice (once with a non-existing .txt file, and once when it already exists) provides the same output for both resolvers. Without the change in the PR, the test fails for the backtracking resolver, but passes for the legacy one.

Contributor checklist
  • Provided the tests for the changes.
  • Assure PR title is short, clear, and good to be included in the user-oriented changelog
Maintainer checklist
  • Assure 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).

@mkniewallner mkniewallner marked this pull request as ready for review October 31, 2022 00:57
@atugushev atugushev added bug Something is not working resolver Related to dependency resolver labels Nov 3, 2022
Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for the fix. Honestly, I don't remember the intention of ireq_key not in self.existing_constraints condition. However, I'm glad the fix works for legacy and backtracking resolvers which is a good signal.

Minor comment below.

tests/test_cli_compile.py Outdated Show resolved Hide resolved
Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

Neat!

@mkniewallner
Copy link
Contributor Author

Not sure why the CI is failing, but it doesn't seem to be related to this PR, as the same error appears on other workflows (https://github.com/jazzband/pip-tools/actions/runs/3382935861/jobs/5618360528#step:10:257).

@atugushev atugushev enabled auto-merge (squash) November 5, 2022 01:19
@atugushev
Copy link
Member

Not sure why the CI is failing, but it doesn't seem to be related to this PR, as the same error appears on other workflows (https://github.com/jazzband/pip-tools/actions/runs/3382935861/jobs/5618360528#step:10:257).

Yeah, that's unrelated to the PR. I'm gonna merge this anyway.

@atugushev atugushev merged commit 77318ca into jazzband:master Nov 5, 2022
@atugushev atugushev added this to the 6.10.0 milestone Nov 11, 2022
dand-oss pushed a commit to dand-oss/pip-tools that referenced this pull request Nov 12, 2022
@mkniewallner mkniewallner deleted the fix/1712-inconsistent-handling-via-backtracking-resolver branch November 25, 2022 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working resolver Related to dependency resolver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent via comments for backtracking resolver on constrained requirements
2 participants