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.

Sign up for GitHub

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

Jump to bottom

Fix compile cached vcs packages #1649

Merged
merged 6 commits into from Oct 1, 2022
Merged

Fix compile cached vcs packages #1649

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a27a8f7
Fix compile cached vcs package
atugushev Jun 30, 2022
9b31c06
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 30, 2022
28bebfd
Pass always candidate.source_link
atugushev Jun 30, 2022
57012f6
Correct test name
atugushev Jun 30, 2022
ccfc3df
Remove test :(
atugushev Sep 30, 2022
5beeb5c
Merge branch 'master' into fix-compile-cached-vcs-1647
atugushev Sep 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion piptools/resolver.py
View file
Open in desktop
Expand Up @@ -763,7 +763,14 @@ def _get_install_requirement_from_candidate(

# Prepare pinned install requirement. Copy it from candidate's install
# requirement so that it could be mutated later.
pinned_ireq = copy_install_requirement(ireq)
pinned_ireq = copy_install_requirement(
template=ireq,
# The link this candidate "originates" from. This is different
# 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!

)

# Canonicalize name
assert ireq.name is not None
Expand Down
33 changes: 33 additions & 0 deletions tests/test_cli_compile.py
View file
Open in desktop
Expand Up @@ -511,6 +511,39 @@ def test_editable_package_vcs(runner):
assert "click" in out.stderr # dependency of pip-tools


@pytest.mark.network
def test_compile_cached_vcs_package(runner):
"""
Test that cached vcs package does not interfere pip-compile results.

Regression test for issue GH-1647.
"""
vcs_package = (
"six @ git+https://github.com/benjaminp/six@"
"65486e4383f9f411da95937451205d3c7b61b9e1"
)

# 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 •
edited

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 •
edited

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 •
edited

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 •
edited

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.


with open("requirements.in", "w") as req_in:
req_in.write(vcs_package)

out = runner.invoke(cli, ["--no-header", "--no-emit-options", "--no-annotate"])

assert out.exit_code == 0, out
assert vcs_package == out.stderr.strip()


@legacy_resolver_only
def test_locally_available_editable_package_is_not_archived_in_cache_dir(
pip_conf, tmpdir, runner
Expand Down