search

pypa / pip

The Python package installer
https://pip.pypa.io/
MIT License
9.51k stars 3.02k forks source link

[Package's VCS dependencies do not build and install when doing a pip install] #10095

Open harshmttl opened 3 years ago

harshmttl commented 3 years ago

Description

When a VCS (git/SVN) package is specified as an 'install_requires' dependency in setup.py, the package is downloaded from the version control, but it is never built or installed. The only way to make it work is right now to issue the --force-reinstall flag. The root cause is a missing check in the loop at https://github.com/pypa/pip/blob/main/src/pip/_internal/resolution/resolvelib/resolver.py#L139.

Candidates such as VCS packages slip through all the checks and land at https://github.com/pypa/pip/blob/main/src/pip/_internal/resolution/resolvelib/resolver.py#L193.

It also explains why --force-reinstall works. The check on line https://github.com/pypa/pip/blob/main/src/pip/_internal/resolution/resolvelib/resolver.py#L150 short-circuits and adds the candidate to the req_set.

The fix is to add a simple check on candidate.source_link and candidate.source_link.is_vcs condition and to set ireq.should_reinstall=True following that.

I suspect this would be broken for all possible mechanisms wherever VCS packages are allowed in requirement spec (requirements.txt, etc.), but I haven't tested those flows.

Expected behavior

Including a VCS package in the dependency list should correctly build and install the dependency much like other regular dependencies.

pip version

Reproduced on pip 21.1.2

Python version

Python 3.9.5

OS

macOS Catalina

How to Reproduce

  1. Create a package with a VCS dependency in install_requires section of setup.py
  2. Run pip install on the target package
  3. Installation succeeds but the VCS package is neither python built (no whl or src dist created) nor installed.

Output

No response

Code of Conduct

harshmttl commented 3 years ago

I can raise a PR for this if the proposed solve seems right. Please suggest.

pradyunsg commented 3 years ago

Thanks for the detailed issue description! Can you provide a minimal reproducer for this?

uranusjr commented 3 years ago

I think --use-feature=in-tree-build (not sure if I remember the name correctly; you should see a warning mentioning this when you pip install) covers this.

harshmttl commented 3 years ago

@uranusjr: It doesn't. This feature works for top level packages and will recursively apply to all collected dependencies as well. The problem here is that the code block I referenced above, doesn't even allow VCS packages to be collected as 'buildable/installble' dependencies. I'll shortly provide a repro case that hopefully makes this clear.

harshmttl commented 3 years ago

Okay, so I figured out that this isn't an entirely broken flow. This breaks only(or rather works as expected but sadly doesn't emit sufficient warnings) when a VCS dependency is already installed. The scenario is that if you have a VCS dependency that has already been installed somewhere in your environment, pip silently swallows that detail and skips the package from being built and installed again.

The behavior is different for already installed wheels or sdist however: https://github.com/pypa/pip/blob/main/src/pip/_internal/resolution/resolvelib/resolver.py#L160

In my case, I had the VCS package built and installed into a local workspace and no matter how many times I did a pip uninstall <<VCS_dependency>>, that pre-built dist was being discovered and pip would skip re-building/re-installing my latest commit from VCS. That should have been generally okay but a) the workspace wasn't in my sys.path and so even though pip install completed happily, at runtime things would blow up, b) I had no easy mechanism to figure out why pip wouldn't build/install my VCS dependency. A simple info/warn message that highlighted that pip is already using an installed version would have given me some leads. Bonus would be to print out the path that it is using, much like how we do it for other regular dependencies, eg: Requirement already satisfied: click_logging in ./vcs-2/lib/python3.9/site-packages (from hsbuildercli==0.0.1) (1.0.1)

We can either a) add a logger/deprecated warning similar to sdist/wheels use-case, b) add a warning but still allow reinstalls for VCS dependencies if pip cannot/doesn't track the last installed commitId/tag.

Also confirmed that this is the same behavior even with --use-feature=in-tree-build flags, so this is definitely something that we should look at fixing.

pradyunsg commented 3 years ago

a VCS dependency is already installed.

Cool, that's what I expected based on a reading of the code -- so it's good to have a confirmation on this. IDK what the right answer here is, so it's gonna be a while before I respond.

FWIW, this is the same problem as the one that lead to us adding those warnings -- we didn't account for this case when we were writing this chunk of code originally, and it's showing through the cracks. :)

pradyunsg commented 3 years ago

@harshmttl Could you confirm that the behaviour between --use-deprecated=legacy-resolver and the default resolver is different in this specific case? And, if it is, what the legacy resolver's behaviour here is.

harshmttl commented 3 years ago

Here's a quick repro:

  1. Clone https://github.com/harshmttl/vcs-parent-PIP10095. This package has a dependency on https://github.com/harshmttl/vcs-deps1-PIP10095 as listed in https://github.com/harshmttl/vcs-parent-PIP10095/blob/main/setup.py#L82
  2. Spin up a virtualenv
  3. Perform pip install .
  4. Run helloVCS command. You should get an output like: 
    Contacting app
Google said 200
  5. Change https://github.com/harshmttl/vcs-parent-PIP10095/blob/main/setup.py#L82 and replace branch name from main to dev
  6. Re-run pip install .
  7. Install logs: 
    Processing /Users/harshmittal/workspace/vcs-parent-PIP10095
DEPRECATION: A future pip version will change local packages to be built in-place without first copying to a temporary directory. We recommend you use --use-feature=in-tree-build to test your packages with this new behavior before it becomes the default.
pip 21.3 will remove support for this functionality. You can find discussion regarding this at https://github.com/pypa/pip/issues/7555.
Collecting helloApp@ git+https://github.com/harshmttl/vcs-deps1-PIP10095@dev
Cloning https://github.com/harshmttl/vcs-deps1-PIP10095 (to revision dev) to /private/var/folders/l1/j7sptcwj3lz5k_7k8p27k1x00000gp/T/pip-install-ap_8tenu/helloapp_2400881847c544708ca8a26da8b705e2
Running command git clone -q https://github.com/harshmttl/vcs-deps1-PIP10095 /private/var/folders/l1/j7sptcwj3lz5k_7k8p27k1x00000gp/T/pip-install-ap_8tenu/helloapp_2400881847c544708ca8a26da8b705e2
Running command git checkout -b dev --track origin/dev
Switched to a new branch 'dev'
Branch 'dev' set up to track remote branch 'dev' from 'origin'.
Requirement already satisfied: requests in ./temp/lib/python3.9/site-packages (from helloApp@ git+https://github.com/harshmttl/vcs-deps1-PIP10095@dev->helloVCS==0.0.1) (2.25.1)
Requirement already satisfied: certifi>=2017.4.17 in ./temp/lib/python3.9/site-packages (from requests->helloApp@ git+https://github.com/harshmttl/vcs-deps1-PIP10095@dev->helloVCS==0.0.1) (2021.5.30)
Requirement already satisfied: urllib3<1.27,>=1.21.1 in ./temp/lib/python3.9/site-packages (from requests->helloApp@ git+https://github.com/harshmttl/vcs-deps1-PIP10095@dev->helloVCS==0.0.1) (1.26.5)
Requirement already satisfied: chardet<5,>=3.0.2 in ./temp/lib/python3.9/site-packages (from requests->helloApp@ git+https://github.com/harshmttl/vcs-deps1-PIP10095@dev->helloVCS==0.0.1) (4.0.0)
Requirement already satisfied: idna<3,>=2.5 in ./temp/lib/python3.9/site-packages (from requests->helloApp@ git+https://github.com/harshmttl/vcs-deps1-PIP10095@dev->helloVCS==0.0.1) (2.10)
Building wheels for collected packages: helloVCS
Building wheel for helloVCS (setup.py) ... done
Created wheel for helloVCS: filename=helloVCS-0.0.1-py3-none-any.whl size=1792 sha256=5b758989ef7dcc077389fb65c004641b0ebfa916bd80517474ed30ab98d07319
Stored in directory: /Users/harshmittal/Library/Caches/pip/wheels/1a/13/f1/986513270b8c8f40cb42c538dddcd0acba9a4ebf260428e4dd
Successfully built helloVCS
Installing collected packages: helloVCS
Attempting uninstall: helloVCS
Found existing installation: helloVCS 0.0.1
Uninstalling helloVCS-0.0.1:
  Successfully uninstalled helloVCS-0.0.1
Successfully installed helloVCS-0.0.1
  8. Re-run helloVCS
  9. Expected Output: 
    Contacting app
Google said 500

    Actual output

    Contacting app
Google said 200
  10. Run pip install . --force-reinstall
  11. This time correctly rebuilds the VCS dependency. Logs show: 
    ...
Building wheels for collected packages: helloVCS, helloApp
Building wheel for helloVCS (setup.py) ... done
Created wheel for helloVCS: filename=helloVCS-0.0.1-py3-none-any.whl size=1792 sha256=28cd7dd06e18dc4c2936f94ea84e91a270ac2d86d96aabba089297a3acd329ed
Stored in directory: /Users/harshmittal/Library/Caches/pip/wheels/1a/13/f1/986513270b8c8f40cb42c538dddcd0acba9a4ebf260428e4dd
Building wheel for helloApp (setup.py) ... done
Created wheel for helloApp: filename=helloApp-0.0.1-py3-none-any.whl size=1570 sha256=49b05f6befb05ab2f32870f09d2b40c1a4fc936bdc43bff6897c55da793774a0
Stored in directory: /private/var/folders/l1/j7sptcwj3lz5k_7k8p27k1x00000gp/T/pip-ephem-wheel-cache-1rquikr4/wheels/fc/cf/76/1559f7de4b21f88fa0e49559eb71d68c577741e5e030a97052
Successfully built helloVCS helloApp
...
  12. The expected output is now shown.
  13. Proves that there is no discernible feedback to the user that an old installation was used and where that installation is. Secondly, treats different commits (branches in this case) as the same package version and doesn't do a force-reinstall.
harshmttl commented 3 years ago

@pradyunsg: legacy resolver is behaving the same as default resolver. Verified with the above repro setup.

pfmoore commented 3 years ago

The main and dev branches of your dependency both have the same name and version. So pip will see that name/version is already installed and not reinstall. This seems to me like a problem with the code, not with pip.

harshmttl commented 3 years ago

Fair point. However:

  1. In this case, shouldn't the branch name be implicitly used as the version indicator?
  2. Even if we don't do a force-update, it'll be still useful to add the logger statements (which indicate explicitly that a reinstall is being skipped) as I've called out in my proposal. Seems like an easy win to avoid accidental developer pain (I alteast spent the last two days debugging this as I've detailed out above).
pfmoore commented 3 years ago

In this case, shouldn't the branch name be implicitly used as the version indicator?

That's not the approach taken anywhere else, so I'd be against special-casing this one situation. Making the uniqueness condition include the branch name globally sounds to me like a much bigger change, so I think it needs more discussion (disclaimer, I'm not that familiar with the rules on how we handle VCS URLs, as they aren't covered by any of the existing standards, so I'm guessing a bit here).

it'll be still useful to add the logger statements

Agreed.

uranusjr commented 3 years ago

In this case, shouldn't the branch name be implicitly used as the version indicator?

That's not the approach taken anywhere else, so I'd be against special-casing this one situation.

+1, and I think there is a reason tools do that. Taking VCS revision name into version consideration basically means having two independent versioning schemes, and all hell breaks loose if they don’t agree with each other. It’s definitely better to go there.

This thread is more or less #5780 but for VCS, so maybe https://github.com/pypa/pip/issues/5780#issuecomment-678676463 would apply as well.

pradyunsg commented 3 years ago

Yep, this is basically #5780.

I'm OK with consolidating this into that issue, with a note of "hey, remember about VCS links when tackling this".