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

Better diagnose when setup.py/cfg cannot be found #9945

Merged
merged 2 commits into from
May 21, 2021

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented May 4, 2021

This adds a check before invoking egg_info to make sure either setup.py or setup.cfg actually exists, and emit a clearer error message when neither can be found.

Fix #9944.

@uranusjr uranusjr requested a review from sbidoul May 4, 2021 08:54
This adds a check before invoking 'egg_info' to make sure either setup.py or
setup.cfg actually exists, and emit a clearer error message when neither can
be found and the egg_info command can never succeed.
@uranusjr uranusjr force-pushed the legacy-egg-info-check-setup-exists branch from 5b14f05 to 1983ef0 Compare May 4, 2021 08:55
@sbidoul sbidoul added this to the 21.1.2 milestone May 9, 2021
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

Thanks @uranusjr

@sbidoul
Copy link
Member

sbidoul commented May 9, 2021

Interestingly, while testing this I noticed that, assuming ./pkgdir contains setup.cfg but no setup.py:

pip install file://$PWD/pkgdir works, but pip install ./pkgdir complains that Neither 'setup.py' nor 'pyproject.toml' found..

The "fix" for that would be easy (in is_installable_dir()), but I'm not sure how far these ripples will go...

@uranusjr
Copy link
Member Author

uranusjr commented May 11, 2021

I grep-ed the code base, is_installable_dir() is only (indirectly) called in parse_req_from_line(), used to parse requirement lines that is not PEP 508 nor looks like a URL. So I think just relaxing the check will be fine.

@sbidoul
Copy link
Member

sbidoul commented May 12, 2021

@uranusjr pre-commit is complaining.

Copy link
Member

@DiddiLeija DiddiLeija left a comment

Choose a reason for hiding this comment

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

@uranusjr I think it can help. It sounds fine. Thanks.

@uranusjr uranusjr force-pushed the legacy-egg-info-check-setup-exists branch from 9f28003 to 1904e4d Compare May 17, 2021 18:48
@sbidoul sbidoul merged commit caf55a1 into pypa:main May 21, 2021
@uranusjr uranusjr deleted the legacy-egg-info-check-setup-exists branch May 21, 2021 21:09
@FFY00
Copy link
Member

FFY00 commented May 26, 2021

Please consider changing this check to only pyproject.toml and setup.py. PEP 517 does not say anything about a project just containing a setup.cfg being valid. We already had this discussion in https://github.com/pypa/build and adapted to better conform to the specification.

cc @pfmoore @henryiii

@henryiii
Copy link
Contributor

Allowing a setup.cfg without a pyproject.toml does seem like a step backward.

For ease of reference:

$ pip wheel .
ERROR: Directory '.' is not installable. Neither 'setup.py' nor 'pyproject.toml' found.
WARNING: You are using pip version 21.1.1; however, version 21.1.2 is available.
You should consider upgrading via the '/Users/henryschreiner/tmp/quickcheck/venv/bin/python3.9 -m pip install --upgrade pip' command.
$ pip install --upgrade pip
$ pip wheel .
Processing /Users/henryschreiner/tmp/quickcheck
  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 numpy>=1.13.3
  File was already downloaded /Users/henryschreiner/tmp/quickcheck/numpy-1.20.3-cp39-cp39-macosx_10_9_x86_64.whl
Building wheels for collected packages: quickcheck
  Building wheel for quickcheck (setup.py) ... done
  Created wheel for quickcheck: filename=quickcheck-0.0.0-py3-none-any.whl size=3372 sha256=57acfb84109eedde21404ffdea6ef0e87d6ef7663498e3220bb0c4a72b066d35
  Stored in directory: /private/var/folders/_8/xtbws09n017fbzdx9dmgnyyr0000gn/T/pip-ephem-wheel-cache-62ksr1uu/wheels/9a/34/3d/e8d9298f424ae357bac078a56990fce39937c7ed9018594f47
Successfully built quickcheck

@uranusjr
Copy link
Member Author

PEP 517 does not say anything about a project just containing a setup.cfg being valid.

I don’t understand. This is about legacy setuptools projects and not related to PEP 517 in any way. A PEP 517 project should not go through any of the changed code paths with or without this change (and if they do, their behavioural changes should be considered a bug).

Allowing a setup.cfg without a pyproject.toml does seem like a step backward.

I don’t understand this either. What is in directory '.'? Why does the behavioural change seem like a step backward?

It seems like there is a big missing link between this PR and things you are complaining about.

@henryiii
Copy link
Contributor

If you create a project with just a setup.cfg (no pyproject.toml, no setup.cfg), up until this PR, pip wheel . would error out with the message above - Directory '.' is not installable. Neither 'setup.py' nor 'pyproject.toml' found. Now it will accept a directory that only has a setup.cfg, and will install it.

@pfmoore added a static check to pypa/build that caused pypa/build to quit if at least one of those two files (the "legacy" setup.py or the "PEP 517" pyproject.toml) was not present. This caused build to suddenly no longer allow a package with only a setup.cfg to work, due to this static check. The argument was it was not correct to expand the check because standards did not mention setup.cfg-only as valid. I was okay with it because pip also followed this rule - until the PR was merged, when suddenly a setup.cfg only project now does build with pip, but not build.

$ tree
.
├── LICENSE
├── README.md
├── setup.cfg
└── src
   └── quickcheck
        └── __init__.py

@@ -269,18 +269,14 @@ def tabulate(rows):
return table, sizes


def is_installable_dir(path):
# type: (str) -> bool
"""Is path is a directory containing setup.py or pyproject.toml?"""
Copy link
Contributor

Choose a reason for hiding this comment

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

This didn't allow setup.cfg only before.

return False
return any(
os.path.isfile(os.path.join(path, signifier))
for signifier in ("pyproject.toml", "setup.cfg", "setup.py")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a logic change! before setup.cfg only didn't pass this check, now it does.

@henryiii
Copy link
Contributor

henryiii commented May 26, 2021

And, relevant messages from the build discussion, where @pfmoore argued we could not add setup.cfg to this check in pypa/build#260 (comment):

If there's no pyproject.toml then (in certain circumstances - it's complicated with all sorts of compatibility cruft) pip will use the setuptools.build_meta:__legacy__ backend, as suggested in PEP 517.

Also, see my comment here. In terms of PEP 517, there are only two sorts of source tree - ones with pyproject.toml and ones with setup.py. Nothing else is technically a Python source tree, if you accept PEP 517's definition.

I was arguing that setup.cfg-only was valid, but pip didn't support it, so I conceded it was okay for build to statically force a directory with only setup.cfg to fail. I'm really fairly neutral at this point, but I want the two tools to do the same thing. I see the logical sense in setup.cfg-only behaving just like setup.py-only, but I also see that we want to be moving toward pyproject.toml always being present; that setup.py works when it is not present is historical.

This is, in both tools, just a check to provide an error message; both tools actually do support setup.cfg only packages if you let them pass this check.

@pfmoore
Copy link
Member

pfmoore commented May 26, 2021

To clarify the position, PEP 517 explicitly requires pyproject.toml ("new style" source tree) or setup.py ("old style" source tree). This is the relevant text:

There is an existing, legacy source tree format involving setup.py. We don't try to specify it further; its de facto specification is encoded in the source code and documentation of distutils, setuptools, pip, and other tools. We'll refer to it as the setup.py-style.

Here we define a new style of source tree based around the pyproject.toml file defined in PEP 518, extending the [build-system] table in that file with one additional key, build-backend.

setup.cfg only trees are only usable as a setuptools-specific extension to PEP 517. There's no means of building such trees without calling a PEP 517 hook, but there's no way of knowing which PEP 517 hook to call without a pyproject.toml to tell you.

However, on re-reading PEP 517, the following paragraph

If the pyproject.toml file is absent, or the build-backend key is missing, the source tree is not using this specification, and tools should revert to the legacy behaviour of running setup.py (either directly, or by implicitly invoking the setuptools.build_meta:legacy backend).

can be interpreted (if we ignore the implication that tools must be able to run setup.py if they wish) as meaning that tools are allowed in that case to call the setuptools legacy backend, rather than invoking setup.py directly, so if that backend works without setup.py, then I guess that's legitimate. But I will note that if we allow that, then in theory we can't validate source trees at all, because it's quite possible for setuptools to decide to support a different mechanism that doesn't even need setup.cfg.

So technically, I stand by the principle that if you only support PEP 517 source trees need setup.py or pyproject.toml to be compliant. But I'd be OK with accepting that as a matter of practicality, allowing trees that only contain setup.cfg is reasonable.

However, I will also point out that the cases for build and pip are very different - build explicitly claims to be a PEP 517 builder, whereas pip goes to great lengths to handle legacy cases. So having build be stricter seems entirely acceptable to me. (Although that's a choice for the build project to make, of course).

This is of course all rules-lawyering. And I don't really have much interest in arguing fine points like this. In reality my view is that all new projects should be using PEP 517, with a pyproject.toml. Older projects which already have a setup.py shouldn't be removing it in favour of setup.cfg only, but should replace it with pyproject.toml if they want to change. So I don't have a lot of interest in supporting the setup.cfg only format anywhere. But that's my personal view, as opposed to my standards maintainer view 🙂

@henryiii
Copy link
Contributor

I've responded in the build issue (pypa/build#302), but in short, the paragraph mentioned above (which is listed in the "changes since acceptance section", so it is "new"ish) clearly means that anything setuptools.build_meta:__legacy__ supports is also valid under PEP 517. Currently, setuptools supports setup.py or setup.cfg, so that would be a practical answer for doing validation on the source tree. If it grew support for something else later, that would also be valid under PEP 517; but I think for practical purposes considering it to be setup.cfg and setup.py is okay, though - anything in the future hopefully will be inside pyproject.toml.

This is just for a check to see if the source tree is valid - a setup.cfg-only project is otherwise completely supported by both pip (>=21.1.1) and build (<0.4.0) since setuptools.build_meta:__legacy__ supports it - it's just this static check, added only to help users who point pip or build at a totally incorrect directory, that causes it not to work.

@FFY00
Copy link
Member

FFY00 commented May 27, 2021

However, I will also point out that the cases for build and pip are very different - build explicitly claims to be a PEP 517 builder, whereas pip goes to great lengths to handle legacy cases.

AFAIK, python -c 'from setuptools import setup; setup()' was never a supported use-case.

Furthermore, I haven't seen anyone asking for pip to support this use-case. The issue this is supposed to fix is related to nothing of the sort.

Emit clearer error message when a project root does not contain either

is the news entry, it does not mention anything about the behavior change.

PEP 517 is designed to be backwards compatible, PEP 517 builders should be able to build all Python projects. Making pip work with setup.cfg only projects will let users to believe that their project is valid, even though it isn't according to PEP 517.

If you don't want to discuss this, then please defer this discussion until someone actually asks for this use-case.

@henryiii
Copy link
Contributor

henryiii commented May 27, 2021

Just to clarify here:

Making pip work with setup.cfg only projects will let users to believe that their project is valid, even though it isn't according to PEP 517.

I think this is technically valid, but with a strong caveat. This is what PEP 517 says (this is in the modifications since release):

Clarified that the setuptools.build_meta:__legacy__ PEP 517 backend is an acceptable alternative to directly invoking setup.py for source trees that don't specify build-backend explicitly.

Due to the "acceptable alternative", that also indicates a perfectly valid PEP 517 builder could manually execute setup.py, and not call setuptools.build_meta:__legacy__ if pyproject.toml is missing - and therefore a setup.cfg-only package is invalid. If setuptools.build_meta:__legacy__ supports setup.cfg without setup.py (which it does), that's why setup.cfg projects do work natively in both build and pip already; it's just this static check that's preventing it from working. PEP 517 differs discussion of what files are required for setuptools.build_meta:__legacy__ to the setuptools package, so anything it supports could "work" when using this alternative. As a practical matter, I think setuptools only is interested in setup.py and setup.cfg packages. But it's the fact that this is an optional alternative that makes PEP 517 not support setup.cfg packages.

This static check was not put in place to limit packages to PEP 517 compliance, but instead to give nice error messages (in build, pip, and cibuildwheel, all have simliar checks).

My preference would be to restore the check here, then maybe discuss adding mention of setup.cfg being valid to PEP 517 if @pypa/setuptools-developers want setup.cfg-only packages to be valid - that would require requiring setuptools.build_meta:__legacy__ over setup.py calls, so not expecting that to happen. However, I do not want to get into a situation where "pip builds my package when developing, but build doesn't when deploying" starts showing up, which could start happening now that build has stopped allowing setup.cfg only and pip has started allowing it. We should be consistent. Development and testing is done with pip, and build is used for deployment. It's important that they stay consistent.

@pfmoore
Copy link
Member

pfmoore commented May 27, 2021

I don't think that anyone anticipated that setuptools.build_meta:__legacy__ would behave differently than setup.py. And in particular we didn't imagine that it would support projects which didn't have setup.py. (The non-legacy backend is another matter, that's intended to be the new PEP 517 only behaviour).

Personally, I view setup.cfg only projects as something that should only be used with the setuptools.build_meta backend (and hence with a pyproject.toml). But I think that's something setuptools should be enforcing, as part of the legacy backend. I think it's legitimate, but not necessary, for front-end tools like build or pip to reject projects that have neither setup.py nor pyproject.toml. I doubt that it's worth raising a bug report against setuptools because the legacy backend is accepting non-legacy project structures, though. That seems pointless unless there's some specific end user problem being caused by the behaviour - and like most Python packaging projects, setuptools already has more work than they can deal with, so I'd prefer to focus on more important issues.

@FFY00
Copy link
Member

FFY00 commented May 27, 2021

I think it's legitimate, but not necessary, for front-end tools like build or pip to reject projects that have neither setup.py nor pyproject.toml.

I disagree. The PEP says

If the pyproject.toml file is absent, or the build-backend key is missing, the source tree is not using this specification, and tools should revert to the legacy behaviour of running setup.py (either directly, or by implicitly invoking the setuptools.build_meta:__legacy__ backend).

which is very clear about running setup.py, setuptools.build_meta:__legacy__ is merely a way to do so. It does not say that setuptools.build_meta:__legacy__ is an alternative to setup.py, it says it is an alternative way of calling setup.py. The result is simple, setup.py should be invoked.

setup.cfg-only projects are not legacy, they are not handled by the PEP, they are a new thing that pip is introducing now. setuptools should not need to enforce anything, because it should never be invoked in such a way.

PEP 517 specifies a new standard for interchangeable build backends, and also specifies how to that interacts with legacy projects and how frontends should behave. pip is deliberately adopting only part of the PEP and saying 'well, I think I know better than the PEP so I am gonna choose how to handle this "legacy" case'. Please don't do this, either implement things how the PEP says, or raise an issue and ask for the PEP to be updated to handle this specific case the way you want. Build frontends should not be behaving in different ways, the PEP is specific how legacy cases should be handled.
I would have an easier time understanding this if this actually solved an issue, but it doesn't, it is just a deliberate behavior choice against the PEP pip is making.

I don't really think I have anything else constructive to add to this discussion. I am not a pip maintainer, so this is not my decision to make, I'll leave you be now.

@idlesign
Copy link
Member

As the original cfg support implementor, I can agree with the point that cfg-only install support should rather be the same in both pip and setuptools, but...
At the implementation phase I was thinking about cfg-only for setuptools (and we even dicussed the issue in setuptools tracker if I'm not mistaken), but getting rid of setup.py wasn't an easy thing to do without an introduction of some addition tool/command and teachin users to use that, so it hasn't been implemented. If we can think about pip as such a tool (and de-facto standard installation mean) support of cfg-only would be nice to get rid of those setup.py's with lonely setup().

@FFY00
Copy link
Member

FFY00 commented May 28, 2021

That is already possible with the mechanism introduced in PEP 517, the question here is if setuptools should have special treatment and be able to bypass the standard mechanism for specifying the backend.

@FFY00
Copy link
Member

FFY00 commented May 28, 2021

I remembered a way to more clearly show how this is wrong. If you do this, setuptools/setup.py is no longer the legacy mechanism but rather it is effectively the default packaging backend.

@henryiii
Copy link
Contributor

From a practical standpoint, moving to setup.cfg-only being distinct from adding a pyproject.toml is nice - convincing someone to adopt one is easier if it is not dependent not the other.

From a purity standpoint, PEP 517 allows a builder to either directly call setup.py or call setuptools.build_meta:__legacy__ if pyproject.toml is not present. Therefore, even though one of the two methods supports setup.cfg-only packages, since the other method does not, it's not really a valid PEP 517 package.

Both pip and build call setuptools.build_meta:__legacy__, so they perfectly happily support setup.cfg without a pyproject.toml - with the exception of a completely unrelated check designed to produce better errors if a user points pip or build at the wrong directory.

Personally, I don't really care that much which is decided on (purity vs. practicality), I just want consistency. I'm done now too, just will wait and see how it plays out.

@uranusjr
Copy link
Member Author

uranusjr commented May 28, 2021

I feel at this point we’re only arguing over semantics without anything actually meaningful. What counts as “the setup.py approach” has never been properly specified, and the PEP avoided specifying it.[1] I’m inclined to think the authors were not really interested in this particular argument in the first place.

Setuptools also explicitly refused to add any build front-end features and told people to get a proper front-end instead, which I took as a sign that front-ends should interpret what “running setup.py” means on our own. pip is therefore free to say setup.cfg-only is a valid legacy project configuration and support it. build is also free to disagree, but PEP 517 should not be used as the reason to convice other projects to accept your interpretation.

[1]: Quoting the PEP, There is an existing, legacy source tree format involving setup.py. We don't try to specify it further; its de facto specification is encoded in the source code and documentation of distutils, setuptools, pip, and other tools. We'll refer to it as the setup.py-style. You can start arguing a setup.cfg-only configuration does not “involve” setup.py and doesn’t count, but again that’s the semantic argument nobody else is intereted in going into.

@pfmoore
Copy link
Member

pfmoore commented May 28, 2021

IMO (as a pip maintainer) the PEP says that we can either call setup.py or call setuptools.build_meta:__legacy__. So I feel we have a right to expect both approaches to do the same thing. So the hook should fail if there's no setup.py, just like calling setup.py (obviously!) does. I feel it's a bug in setuptools that they don't implement that equivalence.

I don't have much sympathy for setuptools saying it's for the front end to deal with this. If we were talking about the non-legacy backend, that would be a different matter, but the legacy backend was explicitly added (in PEP 517) to allow frontends to fall back to the old becahviour while still using a PEP 517 hook. New behaviour should happen on the non-legacy hook.

Because the PR I created for build (as a fix for pypa/build#259) is being quoted as the reason for the checks there, I feel as though I'm expected to be in support of build's behaviour. But in reality I don't care that much¹, all I care about is that "obvious mistakes" are caught. And IMO, that's all that both pip and build should care about. My interest in build's behaviour here is purely as a build user, who was caught out by the lack of any validation. But I've never used a setup.cfg-only project, and I don't intend to, so that edge case doesn't matter to me. See this comment for my summary - which accepts the stricter interpretation of PEP 517, but based on comments by build maintainers @layday and @FFY00, not because it particularly matters to me.

From a pip maintainer's point of view, I'd be fine with accepting a PR that flagged an error if a directory contained only setup.cfg without setup.py or pyproject.toml, directing the user to explicitly specify setuptools in pyproject.toml if they want to omit setup.py. But I'm not inclined to write such a PR myself, as I feel like I've already spent far too much time on this trivial detail.

¹ Even though I don't really care if we warn, flag an error, or just let things fall through to setuptools, I do think that projects that omit setup.py, use setup.cfg, but don't include a pyproject.toml with an explicit reference to the setuptools backend, are wrong and should not be supported. I don't know where the idea came from - I quickly scanned the latest setuptools documentation and the packaging user guide, and neither of them support the practice - but IMO people writing projects in that form should just be told to fix their projects.

@layday
Copy link
Member

layday commented May 28, 2021

The python -c shim is a neat little trick that's a boon to PEP 517 users who want to perform an editable install; but it should not have been applied indiscriminately to everything legacy-related. The validation logic should also not have been relaxed to allow the shim to be used to build wheels. Legacy behaviour should not be changed, precisely because it is legacy. This is just creating a lot of busywork for everyone - trying to figure out if there's a gap in PEP 517, if build should align itself with pip and if the legacy backend should then be invoked - and all for a build system that nobody is invested in anymore.

FFY00 added a commit to FFY00/pip that referenced this pull request May 29, 2021
Per the discussion in pypa#9945.

Signed-off-by: Filipe Laíns <[email protected]>
FFY00 added a commit to FFY00/pip that referenced this pull request May 29, 2021
Per the discussion in pypa#9945.

Signed-off-by: Filipe Laíns <[email protected]>
FFY00 added a commit to FFY00/pip that referenced this pull request May 29, 2021
Per the discussion in pypa#9945.

Signed-off-by: Filipe Laíns <[email protected]>
FFY00 added a commit to FFY00/pip that referenced this pull request May 30, 2021
Per the discussion in pypa#9945.

Signed-off-by: Filipe Laíns <[email protected]>
FFY00 added a commit to FFY00/pip that referenced this pull request May 31, 2021
Per the discussion in pypa#9945.

Signed-off-by: Filipe Laíns <[email protected]>
sbidoul pushed a commit to sbidoul/pip that referenced this pull request Jun 26, 2021
Per the discussion in pypa#9945.

Signed-off-by: Filipe Laíns <[email protected]>
@hroncok
Copy link
Contributor

hroncok commented Jun 26, 2021

IMO (as a pip maintainer) the PEP says that we can either call setup.py or call setuptools.build_meta:__legacy__. So I feel we have a right to expect both approaches to do the same thing. So the hook should fail if there's no setup.py, just like calling setup.py (obviously!) does. I feel it's a bug in setuptools that they don't implement that equivalence.

See also pypa/setuptools#2329

gadomski added a commit to stac-utils/stactools that referenced this pull request Jul 15, 2021
Required with newer pip versions. See
pypa/pip#9945 for some related discussion.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing error when setup.py, setup.cfg and pyproject.toml are absent
9 participants