Skip to content

Treat --upgrade-packages PKGSPECs as constraints (not just minimums), consistently - #1578

Merged
atugushev merged 5 commits into
pypa:mainfrom
AndydeCleyre:bugfix/1550
Dec 12, 2022
Merged

atugushev merged 5 commits into
pypa:mainfrom
AndydeCleyre:bugfix/1550

Conversation

@AndydeCleyre

@AndydeCleyre AndydeCleyre commented Feb 9, 2022 •

Copy link
Copy Markdown
Contributor

Fixes #1550

This aims to address two problems:

  • When --upgrade and --upgrade-packages PKGSPEC are used together, and PKGSPEC's package is not in the input file (but is a subdependency), then that package is upgraded beyond PKGSPEC
  • When --upgrade-packages PKGSPEC is used, and PKGSPEC's package is in neither the input file nor a preexisting output file (but is a subdependency), then that package is upgraded beyond PKGSPEC

In other words:

--upgrade-packages fails to constrain subdeps, if either absent from a preexisting output file, or if --upgrade is also passed

Some of the current behavior was informed by discussion at #759.

However my understanding now is that it's useful, expected, and without significant cost to additionally treat --upgrade-packages specs as constraints, thereby resolving the above cases.

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).

@AndydeCleyre

This comment was marked as outdated.

@AndydeCleyre
AndydeCleyre force-pushed the bugfix/1550 branch 2 times, most recently from afe728c to 6a6b4c7 Compare February 9, 2022 17:58
@AndydeCleyre AndydeCleyre changed the title additional constraints handling for --upgrade-packages additionally treat --upgrade-packages pkgspecs as constraints Feb 10, 2022
@AndydeCleyre AndydeCleyre changed the title additionally treat --upgrade-packages pkgspecs as constraints treat --upgrade-packages PKGSPECs as constraints, even for yet unpinned subdependencies, or when combined with --upgrade Feb 10, 2022
@AndydeCleyre AndydeCleyre changed the title treat --upgrade-packages PKGSPECs as constraints, even for yet unpinned subdependencies, or when combined with --upgrade treat --upgrade-packages PKGSPECs as constraints, even for yet unpinned subdependencies, or when combined with --upgrade Feb 10, 2022
@AndydeCleyre

This comment was marked as outdated.

@AndydeCleyre

Copy link
Copy Markdown
Contributor Author

OK I think the new parametrization for test_upgrade_packages_version_option_and_upgrade covers the first bullet, so I'm going to promote this out of draft state.

@AndydeCleyre
AndydeCleyre marked this pull request as ready for review February 10, 2022 04:58
@AndydeCleyre AndydeCleyre changed the title treat --upgrade-packages PKGSPECs as constraints, even for yet unpinned subdependencies, or when combined with --upgrade treat --upgrade-packages PKGSPECs as constraints, consistently Feb 10, 2022

@richafrank richafrank left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for working to restore this behavior @AndydeCleyre !

Comment thread piptools/scripts/compile.py Outdated
Comment thread piptools/scripts/compile.py Outdated
@AndydeCleyre
AndydeCleyre force-pushed the bugfix/1550 branch 4 times, most recently from cd3002a to e248a40 Compare April 1, 2022 15:32
@AndydeCleyre AndydeCleyre changed the title treat --upgrade-packages PKGSPECs as constraints, consistently Treat --upgrade-packages PKGSPECs as constraints (not just minimums), consistently Apr 1, 2022
@AndydeCleyre
AndydeCleyre force-pushed the bugfix/1550 branch 2 times, most recently from 583104c to e8fd4b0 Compare April 4, 2022 20:44
@AndydeCleyre
AndydeCleyre force-pushed the bugfix/1550 branch 2 times, most recently from e187eaa to efaf7e9 Compare June 30, 2022 21:42
@AndydeCleyre
AndydeCleyre force-pushed the bugfix/1550 branch 2 times, most recently from 06708b0 to 51d4d41 Compare July 18, 2022 15:49
Ensure temporary constraints file gets cleaned up
in a way that avoids Windows file handling complications
Remove leftovers of now unused existing_pins_to_upgrade set
... to cover when the subdependency is not already pinned
... to cover when the -P pkgs are only subdependencies
@atugushev atugushev changed the title Treat --upgrade-packages PKGSPECs as constraints (not just minimums), consistently Treat --upgrade-packages PKGSPECs as constraints (not just minimums), consistently Dec 12, 2022
@atugushev atugushev added the bug Something is not working label Dec 12, 2022

@atugushev atugushev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Finally got to this PR. That looks awesome, thanks!

@atugushev
atugushev enabled auto-merge (squash) December 12, 2022 23:39
@atugushev
atugushev merged commit 6fc9b91 into pypa:main Dec 12, 2022
@atugushev atugushev added this to the 6.12.0 milestone Dec 13, 2022
sirosen added a commit to sirosen/pip-tools that referenced this pull request Feb 6, 2026
When this code was originally written (pypa#481), the tempfile name itself
was being added to the `constraints` list. It therefore needed to persist
until processing of that list was done.

That's no longer the case (the file gets parsed by `pip-tools` itself),
so there's no reason not to use the context manager interface which
guarantees proper cleanup.

In order to test, we need to mock over NamedTemporaryFile, which is
slightly fragile, but better than not having any testing at all.

A similar usage -- without the file leak -- was introduced in pypa#1578, and
is therefore also updated here to use the contextmanager version of
NamedTemporaryFile. Because it was already mostly correct, no new test is
included for that behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something is not working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--upgrade-packages fails to constrain subdeps, if either absent from a preexisting output file, or if --upgrade is also passed

3 participants