jazzband / pip-tools

A set of tools to keep your pinned Python dependencies fresh.
https://pip-tools.rtfd.io
BSD 3-Clause "New" or "Revised" License
7.69k stars 610 forks source link

Remove the requirement that `pip-tools` must be run within a `git` repo #1931

Closed jeffwidman closed 11 months ago

jeffwidman commented 1 year ago

What's the problem this feature will solve?

👋 Hi from the :dependabot: team.

I noticed in the dependabot-core source that pip-tools requires (or at least used to require) running within a git repo:

It's unclear what error the original author was trying to defend against. The only thing I can find with git blame spelunking is this commit:

Given that this commit landed 4 years ago, and a lot has changed with pip-tools since then, I'm also unclear if this is still needed. I will manually test, but even if no obvious error crops up, I still wanted to reach out to see if you, the core maintainer team, might know of silent behavioral changes that are difficult to spot. Or even better you might know the historical context of why pip-tools used to require this, and now doesn't.

If this is still required, then why? Could it be removed?

No other python package manager that :dependabot: supports requires running within a git repo, so it'd be nice to drop this special case handling.

webknjaz commented 1 year ago

Honestly, I'm not sure why it would ever be required. You could probably just remove it on your side and have a "scream test", I suppose...

webknjaz commented 1 year ago

Although, that "if setup files" might imply that it was trying to deal with projects that make sure of things like setuptools-scm that depend on Git to infer their own versions. Then, it makes sense, I suppose.

jeffwidman commented 1 year ago

I tried stopping the git install here:

Which led to this CI failure: https://github.com/dependabot/dependabot-core/actions/runs/5629379712/job/15254272358?pr=6619#step:5:3119

Which is this test failing: https://github.com/dependabot/dependabot-core/blame/8919de6bed26d61cb55e4f05a92c0aca41972233/python/spec/dependabot/python/file_updater/pipfile_file_updater_spec.rb#L371-L400

Which looks like it landed in this commit: https://github.com/dependabot/dependabot-core/commit/d8978397e3f3bc46f22533fbcc3a57a10bdfa637

So the test is checking that if a pipfile is parsed which imports itself, that the setup.py file is successfully parsed... here's the mock pipfiles:

So in :dependabot: we probably are git init'ing a clone of the repo that we're running against so that we can "fake" a package to install from, since some of our parsing logic probably assumes we're not doing a recursive local import.

But what's confusing to me is that we don't do this for any other python package manager... so either there's something unique about pip-tools that results in this situation or it is possible with pip, poetry, etc but we don't handle it properly in :dependabot: ... 🤔

I'm aware of installing a python package as locally editable using pip... it's been several years since I last used piptools... When installing a locally editable package while using piptools, does it still get reflected in the lockfile that way? Or is it now reflected in a different way?

I'll keep digging on this from the :dependabot: side to try to understand what specifically is blowing up to cause the test failure...

webknjaz commented 1 year ago

or it is possible with pip, poetry, etc but we don't handle it properly

Yeah, that was my first hunch. I think that whenever a project with some sort of metadata file, and it's installed using a current directory reference (as in .), it'll need to be built in order for the PEP 517 build backend to calculate the dynamic bits like the project version. I mentioned the setuptools-scm plugin earlier, but other build tools and backends replicate the same way of the dynamic version generation from Git, making it mandatory. An example of that would be hatch-vcs (though hatchling). Such things may make Git a prerequisite build dependency, when building from source. Hence, this is not limited to setup.py.

webknjaz commented 1 year ago

I'm aware of installing a python package as locally editable using pip... it's been several years since I last used piptools... When installing a locally editable package while using piptools, does it still get reflected in the lockfile that way? Or is it now reflected in a different way?

Not sure if this is related, this seems orthogonal in this context…

I'll keep digging on this from the :dependabot: side to try to understand what specifically is blowing up to cause the test failure...

Looks like it's just the expectation of the original author rather than an actual integration test.

terencehonles commented 11 months ago

@jeffwidman this is just a drive by comment so forgive me if I'm mistaken, but are you possibly in the wrong project?

I'm not very familiar with a pipfile, but reading through this thread and looking at the links you specified I think this is a problem with https://github.com/pypa/pipenv or the spec you're referencing(?).

pip-compile produces a requirements.txt that can be used by pip or pip-sync. From the code and comments you linked to in dependabot (and assuming they are correct) it looks like pipenv uses pip-tools, but pipenv is a more complete package manager from what I understand. This means it would likely be the thing pulling in/requiring git.

I was trying to figure out what SHA the pipfiles you were referencing were for. I was initially thinking this might be for requests, but that didn't turn up any matches. I had seen a surprisingly high number of matches when I initially googled, and when not finding the SHA in pipfiles I wanted to make sure I understood the request specifier for editable installs so I could try to reproduce that and hand it to pip-compile. Looking at the docs https://pipenv.pypa.io/en/latest/specifiers/#editable-dependencies-e it looks like that's the package "name" of the current package in the documentation :sweat_smile: so it's not a SHA, or at least not of a commit.

I tried with a minimal Pipfile and Pipfile.lock, and I was unable to reproduce this error, but perhaps it's related to the requirements your lockfile has in it? It's possible one of the packages depends on something that needs git or is the test possibly running in a context where its setup.py/pyproject.toml "uses" git?

webknjaz commented 11 months ago

@terencehonles even if pipenv somehow requires git, my previous points still stand, though. Just to be clear: dependabot runs pip-compile on matching projects, that are not necessarily pipenv-managed. Pipenv is mentioned to augment the context but the question is about pip-tools.

terencehonles commented 11 months ago

As I mentioned this was a bit of a drive by comment, but I spent more time on it than I should have :sweat_smile:, but from what I saw it was hard to tell if that was actually the case, and I think the question was possibly misguided.

I'm doubtful that pipenv uses pip-tools as at least in the time I did spend looking into this it looked like it had its own resolver (and it's not listed in its pyproject.toml or installed when I installed it). I'm not familiar with dependabot's tests, but the tests linked appeared to be failing on a pipenv test. It's quite possible I don't understand the tests and it's actually running both tools and the comment is correct that there is something that pip-tools is breaking on. I just figured I'd ask the OP to verify that assertion that it's an issue with pip-compile rather than pipenv.

To answer the OP's question about what's stored in the lock file for an editable install (the local package depends on Django):

#
# This file is autogenerated by pip-compile with Python 3.11
# by the following command:
#
#    pip-compile
#
-e file:///testing
    # via -r requirements.in
asgiref==3.7.2
    # via django
django==4.2.6
    # via my-package
sqlparse==0.4.4
    # via django

Anyways, I'll exit here since I don't have enough context about dependabot. I've not noticed anything about pip-tools that requires git, so unless they're pinned to a version that did it's not really something I need to answer since I am using the latest version.

webknjaz commented 11 months ago

it's actually running both tools and the comment is correct that there is something that pip-tools is breaking on.

Yes, Dependabot supports many dependency management tools. And they run tests against each. Pipenv was mentioned because of the similarity with pip-tools in that both use Git there.

webknjaz commented 11 months ago

I'm doubtful that pipenv uses pip-tools as at least in the time I did spend looking into this it looked like it had its own resolver

It vendors a patched copy of pip, which, in turn, vendors the depresolution lib called resolvelib: https://github.com/pypa/pipenv/tree/e45cc7a/pipenv/patched/pip/_vendor/resolvelib.

But my point was that the Git requirement must be coming from the PEP 517 backend side of things (which is specific to individual packages) rather than the front-end, which is typically the dependency management solution (pip, pipenv, pip-tools etc.)

terencehonles commented 11 months ago

I don't think we're in disagreement

Pipenv was mentioned because of the similarity with pip-tools in that both use Git there.

pipenv was mentioned because of the comment in their test, if it's a question about package resolution, that should either be in pipenv if it's related to something outside of pip's resolution strategy, otherwise it should be in the pip repository, not here.

It vendors a patched copy of pip, which, in turn, vendors the depresolution lib called resolvelib: https://github.com/pypa/pipenv/tree/e45cc7a/pipenv/patched/pip/_vendor/resolvelib.

yes, but that means it's not using pip-tools which means the issue should not be here.

But my point was that the Git requirement must be coming from the PEP 517 backend side of things (which is specific to individual packages) rather than the front-end, which is typically the dependency management solution (pip, pipenv, pip-tools etc.)

I agree, and I mention this too. This is where it could be a problem with the spec. If they are testing packages that require Git in order to package them and they are editable or don't have a wheel for their CI environment the build system will build a wheel and may pull in other requirements like a build system which requires an SCM/Git.

jeffwidman commented 11 months ago

I'm no longer on the Dependabot team, and while the issue I referenced ☝️ is open source, it's not something I'll be digging into.

Given that it's unclear whether this truly resides in pip-tools, or in some combo of other dependencies with how :dependabot: calls pip-tools, I think it's simplest for now to close this.

We can always re-open later if needed.