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

Refactor parsing setup.py in favour of pip install -e . #908

Closed
atugushev opened this issue Sep 24, 2019 · 17 comments · Fixed by #1311
Closed

Refactor parsing setup.py in favour of pip install -e . #908

atugushev opened this issue Sep 24, 2019 · 17 comments · Fixed by #1311
Labels
enhancement Improvements to functionality needs discussion Need some more discussion setuptools Related to compiling requirements form setup.py

Comments

@atugushev
Copy link
Member

atugushev commented Sep 24, 2019

What's the problem this feature will solve?

The current implementation of pip-compile setup.py doesn't handle the following use-cases and errors:

  1. doesn't support extra requirements, see Pip-compile ignores extras_require when parsing setup.py #625
  2. doesn't support environment markers, see pip-compile with setup.py does not include dependencies with environment markers #1139
  3. doesn't support declarative config setup.cfg
  4. doesn't support modern pyproject.toml config
  5. suppresses an error message when setup.py parse failed, see pip-compile crashes on a setup.py that contains entry_points #572 distutils.core.setup was never called #684
  6. raises an unexpected error during parsing a valid setup.py, see String install_requires fails in parsing #511 In setuptools_markdown: AttributeError: 'NoneType' object has no attribute 'f_code' #695

The solution based on -e . became "de facto" workaround, so why can't we handle it by pip-compile itself?

Describe the solution you'd like

Instead of:

from distutils.core import run_setup
dist = run_setup(src_file)
tmpfile.write("\n".join(dist.install_requires))

get dependencies using something like (consider bash onliner as a demonstration):

$ echo "-e ." | pip-compile - -qo- | sed '/^-e / d' > tmpfile

For example:

$ cat setup.py
from setuptools import setup
setup(
    name="mypackage",
    version="0.1",
    install_requires=['django']
)

$ echo "-e ." | pip-compile - -qo- | sed '/^-e / d'
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile --output-file=- -
#
django==2.2.5
pytz==2019.2              # via django
sqlparse==0.3.0           # via django

Alternative Solutions

Use pip's internal api to get dependencies from setup.py:

...
abstract_dist = pip._internal.legacy_resolver.Resolver._get_abstract_dist_for(req_to_install)

# Parse and return dependencies
dist = abstract_dist.get_pkg_resources_distribution()
...

But i found it challenging to maintain, especially for old pip versions.

Additional context

N/A

UPDATE 2020-05-16: The downside is that -e . solution loses the environment markers.

@atugushev atugushev added feature Request for a new feature needs discussion Need some more discussion enhancement Improvements to functionality and removed feature Request for a new feature labels Sep 24, 2019
@barrywhart
Copy link
Contributor

I believe there is another issue where it currently doesn't handle extras_require. Could this solution be extended to handle that as well? I think it could, for example instead of running pip-compile with -e ., it'd run it with (for example), -e .[extra1,extra2].

@atugushev
Copy link
Member Author

@barrywhart

I think a new cli argument could do it. For example, pip-compile setup.py --extras=extra1,extra2 or pip-compile setup.py --extra=extra1 --extra=extra2.

@ex-nerd
Copy link

ex-nerd commented Nov 5, 2019

That would definitely be helpful. I'm currently bumping up against pip-sync removing all of my packages installed via pip install -e .[dev]

@atugushev
Copy link
Member Author

atugushev commented Nov 25, 2019

UPDATE: added more use-cases to the issue:

  1. doesn't support declarative config setup.cfg
  2. doesn't support modern pyproject.toml config

tchaikov added a commit to tchaikov/teuthology that referenced this issue Dec 4, 2019
we should update this document once
jazzband/pip-tools#625 is fixed and/or
jazzband/pip-tools#908 is merged

Signed-off-by: Kefu Chai <kchai@redhat.com>
tchaikov added a commit to tchaikov/teuthology that referenced this issue Dec 4, 2019
we should update this document once
jazzband/pip-tools#625 is fixed and/or
jazzband/pip-tools#908 is merged

Signed-off-by: Kefu Chai <kchai@redhat.com>
tchaikov added a commit to tchaikov/teuthology that referenced this issue Dec 4, 2019
we should update this document once
jazzband/pip-tools#625 is fixed and/or
jazzband/pip-tools#908 is merged

Signed-off-by: Kefu Chai <kchai@redhat.com>
tchaikov added a commit to tchaikov/teuthology that referenced this issue Dec 5, 2019
we should update this document once
jazzband/pip-tools#625 is fixed and/or
jazzband/pip-tools#908 is merged

Signed-off-by: Kefu Chai <kchai@redhat.com>
kshtsk pushed a commit to kshtsk/teuthology that referenced this issue Dec 11, 2019
we should update this document once
jazzband/pip-tools#625 is fixed and/or
jazzband/pip-tools#908 is merged

Signed-off-by: Kefu Chai <kchai@redhat.com>
@atugushev
Copy link
Member Author

Since we drop the support of old pip versions (see #926) I think better to go for the alternative solution and use pip internals.

@atugushev atugushev added the setuptools Related to compiling requirements form setup.py label Jan 29, 2020
@vphilippon
Copy link
Member

vphilippon commented Jan 30, 2020

EDIT: I wrote this whole thing, skipping this important part of the initial issue (sorry!):

The solution based on -e . became "de facto" workaround, so why can't we handle it by pip-compile itself?

I basically agree on the idea behind that, absolutely.
One question though: why attempt to ommit the -e <path> from the resulting file? The following pip install should still be told to process it (say, for build dependencies, or actually performing some build step), no?

At this point, I'd suggest to work on making piping (|) pip-compile output directly to pip install or pip-sync more user friendly, or something along those line to skip the requirements.in containing just -e ..

Example:
echo "-e ." | pip-compile - -qo- | pip install
or something along those line? It's not so much about keeping a dependency list, rather that benefiting from the dependency resolver until pip has its own.

Original wall-of-text-below, still might be relevant info, will leave it there. Consider it an argument against directly processing setup.py/setup.cfg/pyproject.toml as input.


I'll share what might be an unpopular and purist opinion: I really don't like the fact that we support setup.py as direct input. Over time, I've come to consider it a misfeature.

My purist argument boils down to this article on setup.py vs requirements.txt, or Abstract vs Concrete dependencies, by Donald Stufft (I've brought that up in multiple discussion before, and I will again :) ):
https://caremad.io/posts/2013/07/setup-vs-requirement/

In short: The distribution package is not the app.

You don't need a setup.py file if you're not making a distribution package.
And I believe the same argument applies to setup.cfg and pyproject.toml files.
If this statement sounds wrong to you, please quote and give counter examples! I'd love to learn more.


For the case of the distribution package dev setup, to use pip-tools to manage it you can still think of it as "defining an app using your package".

dev-requirements.in:

-e .[dev]  # See how you don't have any issues specifying your extra?

and now your devs/contributors can run pip-compile dev-requirements.in and pip install -r dev-requirements.txt (or pip-sync dev-requirements.txt).
There's no redundancy here, and you've used a requirements file to define the concrete dependencies (which is its role) for an install.

The point, once again, is to distinguish the distribution package from the thing you want to install, which is a list of concrete dependencies, which includes your package.

When installing your package for dev, you don't run pip install -r setup.py.
No, you run pip install -e ., and I believe that things should go the same way with pip-tools.
You can try to craft a one-liner for doing the equivalent with pip-compile, or we can try to improve pip-tools to facilitate piping (|) pip-compile output to pip install or pip-sync, but using a requirements file stills seems like the cleaner and less error prone technique so far.


TL;DR This feature seems hard to get right (ex: support extras) because it tries to do the wrong job IMO. pip install -r setup.py isn't a thing, and for a good reason.

Should we remove that feature: I'd like to, but we might be doing more wrong than right.
Should we put energy in maintaining, improving and promoting this feature and usage: IMO, No.

@eedwards-sk
Copy link

@vphilippon

You've previously made your opinions / dislike of setup.py known.

As I also previously mentioned, adding the package itself to requirements.in doesn't make sense.

There is considerable value to using setup.py to establish the baseline requirements, along with the requirements file being used for freezing the requirements according to that specific machine / ci workflow.

I feel like the people who don't see the value to setup.py aren't actively making packages that use it.

@vphilippon
Copy link
Member

@eedwards-sk

Thanks for the input, and bringing this previous mention:

As I also previously mentioned, adding the package itself to requirements.in doesn't make sense.

I'm still not sure I see how adding the package itself to requirements.in doesn't make sense.
To the contrary (as I mentioned in my later edit above, sorry about that), the generated requirements.txt should (must?) include a reference the the package itself, no? Otherwise, running pip install -r requirements.txt actually won't process the build steps required by the package itself to be runable afterwards (ex: c++ compilation).

@eedwards-sk
Copy link

eedwards-sk commented Jan 30, 2020

See my last comment in that thread

It's a problem. Not sure why you haven't ran into this yet -- but I don't want my package getting added as a dependency of itself. It makes no sense!

@vphilippon
Copy link
Member

@eedwards-sk

Additionally, -e . does not work with --generate-hashes .

I do not usually use --generate-hashes, and after testing, I see how using -e . and --require-hashes is an issue, you're right.
So unless there's a way for pip-tools to have a path to the package that is hashable (is it even possible at all?), there is an absolute no-go there. TIL.

There is considerable value to using setup.py to establish the baseline requirements, along with the requirements file being used for freezing the requirements according to that specific machine / ci workflow.

Yep. I have a package that should be able to be 'frozen' with hashes, for both its regular dependencies, and foo[dev]. Adding the package itself as a dependency into requirements.txt makes no sense.

Thinking about it over again, I guess I can see some example such as using extras_require to define dev requirements. The fact that there are cases for packages with a setup.py that we actively don't want to process with pip install kind of baffles me, but it looks like it's there.

@atugushev's idea of handling this via an equivalent to -e . internally and remove the package itself make the most sense I guess.
In that case, I'd find it nice to be able to do something like pip-compile .[dev] -o dev-requirements.txt rather than adding a parameter for extras requires, but I feel like this might become troublesome.

@eedwards-sk
Copy link

Thanks for taking the time to learn more about the issue.

@atugushev
Copy link
Member Author

@vphilippon thanks for sharing your thoughts!

In that case, I'd find it nice to be able to do something like pip-compile .[dev] -o dev-requirements.txt rather than adding a parameter for extras requires, but I feel like this might become troublesome.

The idea of pip-compile .[dev] -o dev-requirements.txt sounds good to me. This API looks better than pip-install [setup.py|setup.cfg|pyproject.toml] (see #1047 for setup.cfg and pyproject.toml). I would even use an -e flag for editables to be more consistent with pip, e.g. pip-compile -e .[dev] -o dev-requirements.txt.

@graingert
Copy link
Member

graingert commented Feb 18, 2020

it may be better to use "pep517.meta.load" from pip install pep517

@astrojuanlu
Copy link
Contributor

One comment from the peanut gallery:

I think better to go for the alternative solution and use pip internals.

pip developers are generally not happy with others doing this. pip does not have an API, the devs have repeatedly stated so in several places [citation needed] and regardless, any solution regarding on pip internals will be very fragile.

@graingert
Copy link
Member

@astrojuanlu pip-tools makes heavy use of pip internals already

@atugushev
Copy link
Member Author

The downside of using -e . is that it loses the environment markers. I've updated the issue.

@astrojuanlu
Copy link
Contributor

it may be better to use "pep517.meta.load" from pip install pep517

@jaraco agrees with @graingert

pypa/setuptools#1951 (comment)

And it seems to retain environment markers.

astrojuanlu added a commit to astrojuanlu/pip-tools that referenced this issue Feb 13, 2021
astrojuanlu added a commit to astrojuanlu/pip-tools that referenced this issue Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to functionality needs discussion Need some more discussion setuptools Related to compiling requirements form setup.py
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants