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

Include pyproject.toml in sdist #1650

Merged
merged 9 commits into from
Dec 31, 2019
Merged

Conversation

jaraco
Copy link
Member

@jaraco jaraco commented Jan 27, 2019

This is a re-submit of #1634, which was backed out due to an unexpected regression.

Closes #1632

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details

@jaraco
Copy link
Member Author

jaraco commented Jan 27, 2019

This should be deferred until there's a solution for #1642 and #1644 (or at least tests should pass).

@pganssle
Copy link
Member

pganssle commented Feb 6, 2019

We should probably defer this until at least pip 19.1, because this will start opting people in to PEP 517 in their sdists who were never opted in before. Right now that's a backwards incompatible change, but when we get things more ironed out it hopefully won't be.

@gaborbernat
Copy link
Contributor

Is this still deferred?

@pganssle
Copy link
Member

pganssle commented Aug 8, 2019

@gaborbernat Yeah. As it stands now pip doesn't support the bootstrapping parts of PEP 517, so setuptools cannot be built with PEP 517.

I also suspect that making this change will break many things because, right now, there are a decent number of projects out there that will break if they use PEP 517 in some situations, and they may be relying on the fact that they are not including the pyproject.toml in their sdist.

We should probably defer this until PEP 517 is more stable, possibly until PEP 517 is the default for all builds.

@jaraco
Copy link
Member Author

jaraco commented Sep 11, 2019

In that case, let's close it and we can revive it later if needed.

@jaraco jaraco closed this Sep 11, 2019
@gaborbernat
Copy link
Contributor

I'm not sure I understand what this means? More stable, default? It's already out for at least a year and the default build mechanism in pip for 6 months. In my view no better time than today.

@con-f-use
Copy link
Contributor

con-f-use commented Sep 12, 2019

Agreed, if " a decent number of projects [...] may be relying on the fact that they are not including the pyproject.toml in their sdist", then they are relying on faulty behavior. That shouldn't deter setuptools from doing it right.

Especially since this does not seem to work:

from setuptools import setup

setup(
    package_data={'': ['pyproject.toml']},
    # ...
)

@jaraco
Copy link
Member Author

jaraco commented Dec 29, 2019

As it stands now pip doesn't support the bootstrapping parts of PEP 517, so setuptools cannot be built with PEP 517.

That issue was addressed in #1927.

I also suspect that making this change will break many things because, right now, there are a decent number of projects out there that will break if they use PEP 517 in some situations, and they may be relying on the fact that they are not including the pyproject.toml in their sdist.

I'm not sure I understand what this means? More stable, default? It's already out for at least a year and the default build mechanism in pip for 6 months. In my view no better time than today.

Agreed, if " a decent number of projects [...] may be relying on the fact that they are not including the pyproject.toml in their sdist", then they are relying on faulty behavior. That shouldn't deter setuptools from doing it right.

I agree also. It should be possible to make progress without waiting (essentially forever) for usage of the old behavior to drop to zero.

I notice that setuptools excludes pyproject.toml. Is that a viable workaround for projects that wish to retain this undesirable behavior?

@jaraco
Copy link
Member Author

jaraco commented Dec 29, 2019

Is that a viable workaround for projects that wish to retain this undesirable behavior?

Tentatively confirmed - when building setuptools sdist against this PR, it does not include pyproject.toml, suggesting that the exclude directive in the manifest is a sufficient workaround to suppress the default inclusion.

I'm inclined to proceed with this PR.

@jaraco jaraco reopened this Dec 29, 2019
@jaraco jaraco requested a review from pganssle December 29, 2019 18:02
@pganssle
Copy link
Member

@jaraco I think we might need more details here. This is yet another case where having a docker container or some other mechanism for making a change and then installing the top 100 packages on PyPI would be very helpful. If we do this and it breaks source installs of one of a popular but infrequently-released package on PyPI, we're going to get a lot of people pinning setuptools indefinitely.

One possible "soft launch" version of this is to start by only including pyproject.toml if the [build-system] key is detected. We're likely going to want to vendor the toml library anyway to solve #1688, but if we really don't want to pull it in, we might get away with an even simpler heuristic like re.sub('\s+', '', line) == '[build-system]' (this would miss keys defined not in their own section, like build-system.requires="blah", but I think that is a rare situation).

Things that break when installed via PEP 517 are somewhat rare, but most of the complaints come from people who are not using pyproject.toml to specify build information but rather are using some tool like black or towncrier. In the long run I think we need to include it by default, but possibly only including it when it contains the build information would break fewer things?

If we could test both changes against a random smattering of PyPI projects I'd probably be able to form a stronger opinion here.

@uranusjr
Copy link
Member

uranusjr commented Dec 29, 2019

Does the filelist logic allow for something like “include pyproject.toml by default, unless the user lists it as exclude in MANIFEST.in”? If so it’d be possible to tell packages to explicitly exclude it if they really want to.

@pganssle
Copy link
Member

@uranusjr IIRC that is how exclude works, and if it doesn't, we can certainly make it work that way for anything new added to the default include list.

@pganssle
Copy link
Member

So I was thinking about this and I realized that I had the people this could affect wrong. I was thinking that this change might break projects already uploaded to PyPI when built from source distributions, but this will only change new source distributions built with setuptools.

Since there's no chance of it hitting anyone installing from existing PyPI distributions, the most likely "this causes breakage" scenario is that someone cuts a release that includes pyproject.toml which then breaks a bunch of people installing from sdist. This means that the bugs will mostly be in projects with recent releases, which means that those projects are more likely to be responsive with fixes (and importantly we won't start breaking stuff that worked before - people can just pin to earlier versions of the library).

I think we can probably skip the "only include it if it has a build-system logic, even though it may cause some short term pain.

The main thing we really have to worry about is people who blacklist pyproject.toml today and then later actually add a build backend and forget to take it off the MANIFEST.in excludes list, but I think that's a much smaller cohort than the people who won't know to put it on there in the first place, so I'm +1 for adding it by default.

@jaraco jaraco removed the deferred label Dec 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pyproject.toml should be included in sdist by default
6 participants