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 compile cached vcs packages #1649

Merged
merged 6 commits into from Oct 1, 2022

Conversation

atugushev
Copy link
Member

@atugushev atugushev commented Jun 30, 2022

Fixes #1647

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

@atugushev atugushev added bug fix resolver Related to dependency resolver labels Jun 30, 2022
@atugushev atugushev added this to the 6.8.1 milestone Jun 30, 2022
# from ``ireq.link`` when the link is found in the wheel cache.
# ``ireq.link`` would point to the wheel cache, while this points
# to the found remote link (e.g. from pypi.org).
link=candidate.source_link,
Copy link
Contributor

Choose a reason for hiding this comment

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

Glad your copy_install_requirement extensibility made this easy!

Copy link
Member Author

Choose a reason for hiding this comment

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

That's very handy indeed!

Comment on lines 526 to 536
# Install and cache VCS package
# TODO: install package in isolated environment
subprocess.run(
[
sys.executable,
"-m" "pip",
"install",
vcs_package,
],
check=True,
)
Copy link
Member

@gschaffner gschaffner Jul 2, 2022

Choose a reason for hiding this comment

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

On my machine, this doesn't cause a wheel for six to be cached in ~/.cache/pip/wheels. I did a quick sanity check with [sys.executable, "-m", "pip", "cache", "dir"] here to confirm that ~/.cache/pip/wheels is the relevant cache on my machine when running under tox -e py310-piplatest-coverage.

I had a similar issue when writing the MWE script in #1647. Pip would not cache the wheel unless I installed another version of the VCS package first.

Doing that doesn't seem to fix the issue here though. On my machine, replacing typeguard @ ... with six @ git+https://github.com/benjaminp/six@65486e4383f9f411da95937451205d3c7b61b9e1 in my MWE script doesn't actually cause the issue to occur—for some reason I can't get Pip to cache a wheel for this version of six.

Copy link
Member

@gschaffner gschaffner Jul 2, 2022

Choose a reason for hiding this comment

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

Suggested changes atugushev/pip-tools@fix-compile-cached-vcs-1647...gschaffner:pip-tools:fix-compile-cached-vcs-1647.

bb2e91edbd3d0f1bdd86c41e84c88621f8d35690 is failing—I can't find a way to get Pip to cache six-1.16.0-py2.py3*.whl. I am not sure why1, but the easiest fix is probably to change to another package that supports 3.7–3.10, has no dependencies for this range of Pythons, and that Pip will cache a wheel for, e.g. Plumbum 1.7.2. As far as I can tell, there isn't another candidate for this that's already used in pip-tools's suite elsewhere (the list of git+'s used elsewhere seems to be pip-tools, django, pip, and billiard).

1: My guess is that this is because six does not use setuptools_scm, but I haven't looked into this.

Copy link
Member Author

@atugushev atugushev Jul 6, 2022

Choose a reason for hiding this comment

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

The test works on CI. As you can see it fails without the fix:

AssertionError: assert 'six @ git+ht...05d3c7b61b9e1' == 'six @ file:/...-none-any.whl'
  - six @ file:///Users/albert/Library/Caches/pip/wheels/b4/42/6d/bab90ef8caaba6e66b7c1b7e35d83fbe1b67ae9a1101839ff2/six-1.16.0-py2.py3-none-any.whl
  + six @ git+https://github.com/benjaminp/six@65486e4383f9f411da95937451205d3c7b61b9e1

I think that should be enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gschaffner what would you think?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delay!

Without the fix, the test mistakenly passes on my laptop when run as

$ git checkout 53f36d88bbaaad3ece3e964b015015690c6792e9  # or 835b7e58c4fb929e114d88fce7e1f911aef2c4cd
$ rm -r ~/.cache/pip/wheels
$ tox -e py310-piplatest-coverage -- --no-cov tests/test_cli_compile.py::test_compile_cached_vcs_package  # or tox -e py310-piplatest-coverage

Can you replicate? It seems like the test is not putting six into the cache.

(My initial guess was that the test was failing (as it should) on GHA because six was already in the wheels cache loaded by

https://github.com/jazzband/pip-tools/blob/53f36d88bbaaad3ece3e964b015015690c6792e9/.github/workflows/ci.yml#L61-L68

This doesn't seem to be the case however—see that there is no wheel for six loaded in https://github.com/jazzband/pip-tools/runs/7439890847?check_suite_focus=true#step:7:223.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but isn't it all fine if you just replace six with any other package?

Copy link
Member Author

Choose a reason for hiding this comment

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

That was my first thought. However, while the test works on CI, it can mistakenly pass in a different setup. This makes me think that the test is flaky and must be reworked in a way as suggested here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks, yeah I'm on board to get the fix out ahead of the test.

Copy link
Member Author

@atugushev atugushev Sep 30, 2022

Choose a reason for hiding this comment

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

Tracking issue #1686.

Copy link
Member Author

Choose a reason for hiding this comment

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

@AndydeCleyre okay, all set.

Copy link
Member

@gschaffner gschaffner left a comment

Choose a reason for hiding this comment

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

LGTM. If it's helpful, I'd be happy open a PR for #1686 based on 6f0d7dc.

@atugushev atugushev merged commit 40d4818 into jazzband:master Oct 1, 2022
@atugushev atugushev deleted the fix-compile-cached-vcs-1647 branch October 1, 2022 01:42
@atugushev
Copy link
Member Author

@gschaffner thanks! Feel free to.

@gschaffner gschaffner mentioned this pull request Oct 1, 2022
4 tasks
@atugushev atugushev modified the milestones: 6.8.1, 6.9.0 Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolver Related to dependency resolver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@ git+ requirements with backtracking resolver can result in pins to local wheel cache
4 participants