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 detection of invalid __legacy__ PEP 517 projects #10534

Closed
wants to merge 2 commits into from

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Oct 2, 2021

Fixes #10531

TODO

  • test
  • news

Copy link
Member

@FFY00 FFY00 left a comment

Choose a reason for hiding this comment

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

I think this test should happen before build-backend is set. If you invoke pip on something that it not a Python package, it will still say File 'setup.py' not found for legacy project ..., instead of telling the use that the specified path/url is not a project.

Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

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

Should we include a test for this?

@sbidoul
Copy link
Member Author

sbidoul commented Oct 2, 2021

Should we include a test for this?

Of course :)

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

I don't think we should take this approach here, as I've just elaborated in the corresponding issue.

I'm also not going to spend any energy on this weekend though -- I'd rather get 21.3 ready, because that's much more relevant and useful. :)

@pradyunsg pradyunsg dismissed their stale review October 2, 2021 17:49

Oh nvm, I misread. 😅

@@ -174,4 +174,9 @@ def load_pyproject_toml(
backend = "setuptools.build_meta:__legacy__"
check = ["setuptools>=40.8.0", "wheel"]

if backend == "setuptools.build_meta:__legacy__" and not has_setup:
Copy link
Contributor

Choose a reason for hiding this comment

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

setup.py-less projects should be permitted if they specify a pyproject.toml

Copy link
Member Author

Choose a reason for hiding this comment

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

In which case they don't use the legacy backend, or ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or what if a project lists setuptools.build_meta:__legacy__ as their build-backend? setup.py-less builds should definitely work then

Copy link
Member

@pradyunsg pradyunsg Oct 2, 2021

Choose a reason for hiding this comment

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

And if they don't, why is that a problem? They're explicitly opting into legacy behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right but setuptools.build_meta:__legacy__ automatically generates a setup.py if it's missing

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

@pradyunsg pradyunsg Oct 3, 2021

Choose a reason for hiding this comment

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

I'm aware of the behaviour of setuptools, and that isn't an answer to the question asked.

Let me rephrase: Why should "setup.py-less" projects work when they're using the setuptools.build_meta:__legacy__ backend (whether implicitly, or through an explicit specification in the pyproject.toml file)?

Copy link
Member

Choose a reason for hiding this comment

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

Note that there's a very straightforward solution in those cases -- add the right pyproject.toml content.

@sbidoul sbidoul marked this pull request as draft October 2, 2021 22:26
@sbidoul sbidoul force-pushed the fix-10531-sbi branch 2 times, most recently from a64c732 to a0e4d16 Compare October 3, 2021 10:48
@sbidoul
Copy link
Member Author

sbidoul commented Oct 3, 2021

Or we could always reject projects that have neither a pyproject.toml nor a setup.py (PR updated to do that).
But this would prevent editable install of setup.cfg-only projects, which pip started accepting in 21.2...

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Oct 9, 2021
@pradyunsg
Copy link
Member

pradyunsg commented Oct 10, 2021

But this would prevent editable install of setup.cfg-only projects, which pip started accepting in 21.2...

I think I'm mildly in favour of keeping this. It's a low-effort way for users to move to completely declarative setups (especially until setuptools adds PEP 621 [project metadata in pyproject.toml] support) -- having editable installs work with that is a nice-to-have.

FWIW, this whole thing will involve user-facing behaviour changes, so we'd need a deprecation cycle -- I'm in favour for that being expidited and only keeping it deprecated for a single release though. :)

@sbidoul
Copy link
Member Author

sbidoul commented Oct 10, 2021

@pradyunsg to be clear, projects having a pyproject.toml + setup.cfg would still work. Only projects with setup.cfg and no setup.py nor pyproject.toml would refuse to install.

@layday
Copy link
Member

layday commented Oct 10, 2021

I think it's okay to treat setup.cfg-only projects as invalid in all circumstances. If you can install a package as editable with just a setup.cfg but you can't build it or install it regularly, that's only bound to confuse novices.

@pradyunsg
Copy link
Member

Fair enough. This still needs a deprecation cycle though, because I do imagine folks showing up with pitchforks, because they can no longer use their cute setup.cfg-only projects.

@sbidoul
Copy link
Member Author

sbidoul commented Oct 10, 2021

This still needs a deprecation cycle though, because I do imagine folks showing up with pitchforks, because they can no longer use their cute setup.cfg-only projects.

This has only existed since 21.2, and the solution is touch pyproject.toml, so maybe not ?

@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Oct 10, 2021
@pfmoore
Copy link
Member

pfmoore commented Oct 10, 2021

I agree with @sbidoul - I'd be comfortable treating this as a bug introduced in 21.2 and therefore this is a bugfix. We didn't document "pip will install projects that have setup.cfg but neither setup.py nor pyproject.toml" as a new feature in 21.2, after all...

@pradyunsg
Copy link
Member

pradyunsg commented Oct 10, 2021

From https://pip.pypa.io/en/stable/news/#v21-1:

Add support for editable installs for project with only setup.cfg files. (#9547)


only existed since 21.2 / a bug introduced in 21.2

This was introduced in 21.1.

We didn't document

This was "documented" in the changelog. This is definitely one of the ways we communicate about features, so I certainly consider this to need a deprecation cycle.

the solution is touch pyproject.toml

Only if your project does not have any other build-time dependencies. If you have build-time dependencies that you just install in your build environment, and/or if you're sensitive to what headers you're compiled against (eg: numpy's headers), then this doesn't work.

@pradyunsg
Copy link
Member

pradyunsg commented Oct 10, 2021

To be clear, I'm not advocating to keeping the setup.cfg-only-works behaviour anymore -- but rather, that we should remove it with a deprecation cycle. I'm on board for only having the deprecation for a single release cycle as well.

@graingert
Copy link
Contributor

graingert commented Oct 10, 2021

Only if your project does not have any other build-time dependencies. If you have build-time dependencies that you just install in your build environment, and/or if you're sensitive to what headers you're compiled against (eg: numpy's headers), then this doesn't work.

I thought setup.cfg-only projects only worked under --use-pep517? And so touch pyproject.toml does happen to always work in this case

@pfmoore
Copy link
Member

pfmoore commented Oct 10, 2021

This was introduced in 21.1.

Ah, OK. I took "since 21.2" as "introduced in 21.2" and while I skimmed the earlier changelog, I missed it. My bad. As we did state the behaviour (even if only in the changelog) then my suggestion of thinking of it as a bugfix is wrong. A single release cycle of deprecation sounds like a reasonable compromise.

@sbidoul
Copy link
Member Author

sbidoul commented Oct 10, 2021

I'm back after testing 96 combinations with pip 21.4, this PR and the build project 😓

The following table shows the combinations that behave differently between 21.2.4 and this PR, and also python -m build (look for lines with a ! next to the line number):

# pyproject.toml setup.py setup.cfg editable --use-pep517 file:// pip 21.2.4 this PR build comment
13! True False False True None True success error PEP660 not supported
29! True False False True True True success error PEP660 not supported
57 False False True True None True error error legacy setup.cfg only rejected
59 False False True False None True error error legacy setup.cfg only rejected
73! False False True True True True success error pip install --use-pep517 --editable
74! False False True True True False success error pip install --use-pep517 --editable
75! False False True False True True success error error We fix #10531
76 False False True False True False error error error ok
77! False False False True True True success error not a project
79! False False False False True True success error error not a project

[update]: here is the full table.
[update 2]: clarified comment for lines 13 and 29 (PEP 660 not supported by default build backend)

The table shows that the cases that did succeed before and now fail are all quite exotic.

Notably, pip install -e . of a directory containing only a setup.cfg did fail in 21.2.4 and also fails with this PR.

The only change that could possibly be considered backward-incompatible is that pip install --use-pep517 --editable for a setup.cfg only project with no pyproject.toml now fails. Honestly it would never have occured to me to combine --use-pep517 and --editable to force a setup.cfg-only project to install in editable mode!

So yeah this is such an exotic case that my motivation to do a deprecation cycle for that is nil. But I'm not going to block anyone else willing to do it of course.

@sbidoul sbidoul marked this pull request as ready for review October 10, 2021 14:22
@sbidoul
Copy link
Member Author

sbidoul commented Oct 10, 2021

Also I verified that the same combinations are accepted or rejected by pip and build.

@sbidoul sbidoul added this to the 21.3 milestone Oct 10, 2021
@sbidoul
Copy link
Member Author

sbidoul commented Oct 10, 2021

I tentatively assigned to 21.3. Feel free to drop it if you are not comfortable with it, @pradyunsg

@pradyunsg
Copy link
Member

What do the numbers in the table mean?

@sbidoul
Copy link
Member Author

sbidoul commented Oct 10, 2021

What do the numbers in the table mean?

Nothing. They are just row numbers for easier reference.

@pradyunsg pradyunsg modified the milestones: 21.3, 22.0 Oct 11, 2021
@pradyunsg
Copy link
Member

I've kicked the can down the road for this, mostly because... I don't feel a 100% comfortable just merging this, especially just a few hours before the release! (which is... well... when I realised that, oh, there's another open PR in the milestone 😅)

@sbidoul
Copy link
Member Author

sbidoul commented Oct 12, 2021

Hm, while investigating a fix for #10573 I noticed that while implementing PEP 660 I accidentally allowed pip install -e . for a setup.cfg only project, which was not allowed in 21.2... sorry about that.

The good news is this PR fixes that.

@sbidoul
Copy link
Member Author

sbidoul commented Oct 13, 2021

I'm merging this into #10577

@sbidoul sbidoul closed this Oct 14, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
8 participants