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

pip-compile -P PKGSPEC ignores pkgs not already parsed as constraints #1031

Merged
merged 4 commits into from
Jan 21, 2020

Conversation

AndydeCleyre
Copy link
Contributor

@AndydeCleyre AndydeCleyre commented Jan 17, 2020

Fixes #759

pip-compile -P <pkgspec>

When adding requested-to-upgrade pkgspecs (ireqs) to the constraints list, omit those whose key is not present among the primary_packages set.

Changelog-friendly one-liner: pip-compile --upgrade-package silently ignores those passed packages not already required according to the *.in and *.txt files.

Contributor checklist
  • Provided the tests for the changes.
  • Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@codecov
Copy link

codecov bot commented Jan 17, 2020

Codecov Report

Merging #1031 into master will increase coverage by 0.22%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1031      +/-   ##
==========================================
+ Coverage   99.11%   99.33%   +0.22%     
==========================================
  Files          34       34              
  Lines        2360     2397      +37     
  Branches      302      306       +4     
==========================================
+ Hits         2339     2381      +42     
+ Misses         11        8       -3     
+ Partials       10        8       -2
Impacted Files Coverage Δ
tests/test_cli_compile.py 100% <100%> (ø) ⬆️
piptools/scripts/compile.py 100% <100%> (ø) ⬆️
piptools/repositories/pypi.py 95.6% <0%> (+2.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6709244...2f0d80a. Read the comment docs.

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.

Hello @AndydeCleyre,

Thanks for the PR! Looks like these changes introduce a bug when pip-compile wouldn't upgrade/downgrade a secondary package to a specific version, see:

$ cat requirements.in
jinja2

$ cat requirements.txt
jinja2==2.10.3
markupsafe==1.0           # via jinja2

$ pip-compile --no-header -P markupsafe==1.1.0
jinja2==2.10.3
markupsafe==1.1.1         # via jinja2

Note that markupsafe has upgraded to the latest 1.1.1 version (not 1.1.0 as expected).

@AndydeCleyre
Copy link
Contributor Author

Thanks!

I didn't think about user-managed versions of requirements not entered into the .in file. Some options:

a) alter the resolver to handle an additional constraint variant which limits only if needed
b) run the resolver twice in the case that upgrade_install_reqs isn't empty, the second time without those constraints added
c) only support -P for reqs.in entries

@atugushev
Copy link
Member

atugushev commented Jan 18, 2020

@AndydeCleyre

d) this also can be fixed by:

if not upgrade and os.path.exists(output_file.name):
    ...
    existing_pins_to_upgrade = {
        key_from_req(ireq.req): ireq
        for ireq in ireqs
        if is_pinned_requirement(ireq)
        and key_from_req(ireq.req) in upgrade_install_reqs
    }
...
constraints.extend(
    ireq for key, ireq in upgrade_install_reqs.items()
    if key in primary_packages or key in existing_pins_to_upgrade
)

But this fix doesn't handle the case when a user removes a package from requirements.in and wants to upgrade the removed package before recompiling the requirements.txt. We may ignore that because we can't protect people from shooting to the foot.

@AndydeCleyre
Copy link
Contributor Author

Alright, so after your last comment I implemented your fix only slightly modified by populating existing_pins_to_upgrade and existing_pins during a single iteration through ireqs.

But it drew my attention to the fact that I wasn't yet familiar with LocalRequirementsRepository, and then I thought the distinction between pinned and to-upgrade was unnecessary, so that a better solution would be to stop separating them, and just make sure to update the existing pins with the to-upgrade reqs (and never directly add upgrade_install_reqs to the constraints:

existing_pins = {
    key_from_req(ireq.req): ireq
    for ireq in ireqs
    if is_pinned_requirement(ireq)
}
existing_pins.update(upgrade_install_reqs)

At first I thought it was working, but then realized that it breaks when using -P with specifiers other than ==, as <, <=, etc., aren't "pinned." And converting one of those to a true pin isn't so simple, as it entails using the resolver and accessing the real repository.

So I've replaced this PR content with the modified version of your last-suggested solution.

I tried to add a little extra to test_upgrade_packages_version_option to cover this, but I don't sufficiently understand the way the test_data fake packages with deps are handled to get it right. If the fix looks good, can you add a test for that case?

I still keep in my head as a fallback for "correct" (if inefficient) outcome: in the case of upgrades, resolve/pin/compile once with all constraints, and again without the to-upgrade ones.

@AndydeCleyre
Copy link
Contributor Author

Ah, the star unpacking isn't py2 compatible, I'll update shortly.

@atugushev
Copy link
Member

@AndydeCleyre

Thanks for the updates! I've skimmed the PR but review it properly soon.

Here is the PR AndydeCleyre#1 with tests. Also, I've added a test for downgrade deps.

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.

Looks good! A few minor comments.

piptools/scripts/compile.py Outdated Show resolved Hide resolved
tests/test_cli_compile.py Outdated Show resolved Hide resolved
tests/test_cli_compile.py Outdated Show resolved Hide resolved
…exclude -P args not already pinned or primarily required; fixes jazzband#759

Co-Authored-By: Albert Tugushev <[email protected]>

use long option style --upgrade-package in test

exact line-matching in expected output

more readable if/else instead of ternary
@AndydeCleyre
Copy link
Contributor Author

You're totally right about it naturally being a set rather than a dict, that was just a holdover from trying to more tightly combine the population of existing_pins and existing_pins_to_upgrade, which we scrapped anyway.

Thank you for your continued work on this project, and ensuring I don't muck it up.

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! 🎉 🎉 🎉

@atugushev atugushev added this to the 4.4.0 milestone Jan 20, 2020
@atugushev atugushev merged commit 9e01b6e into jazzband:master Jan 21, 2020
@atugushev
Copy link
Member

@AndydeCleyre thanks!

@atugushev
Copy link
Member

pip-tools v4.4.0 is released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pip-compile upgrades a package even it doesn't exist in a requrements.*
2 participants