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

[AndydeCleyre]

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]

Codecov Report

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

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

[atugushev]

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]

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]

@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]

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]

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

[atugushev]

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

[atugushev]

Looks good! A few minor comments.

[AndydeCleyre]

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.

[atugushev]

Awesome! 🎉 🎉 🎉

[atugushev]

@AndydeCleyre thanks!

[atugushev]

pip-tools v4.4.0 is released!