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

Patched sdist in PEP 517 to include pyproject.toml #1949

Closed

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Dec 27, 2019

Summary of changes

A flag is added to setuptools.command.sdist.sdist to include pyproject.toml if present. This flag is False by default, but set to True in PEP 517 build hooks, so the built sdist is compliant to the PEP.

Non-PEP 517 sdist builds remain the same as before (not including pyproject.toml unless explicitly included in MANIFEST.in).

Ref #1632

Pull Request Checklist

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

@uranusjr uranusjr force-pushed the build-sdist-include-pyproject-toml branch from ddf3c84 to 7d352dc Compare December 27, 2019 08:29
Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

Thanks for proposing this change.

I looked back at why pyproject.toml couldn't be included by default with non PEP 517 sdist builds, and it seems to be that the presence of pyproject.toml causes builds of those sdists to fail for some projects (or only setuptools, possibly).

Given that this change will cause the pyproject.toml to be present in some sdists, how can we be confident that this change won't have the same regression as #1634?

Overall, I'm lukewarm on this idea. It adds complexity to the build system and inconsistency across build modes, yet only achieves half of the intended change (to support pyproject.toml). I'd really rather have a more general solution to the pyproject.toml problem.

If the regression only applies to setuptools building itself, perhaps the exclusion could be added just for when setuptools is being built (and include it by default otherwise).

return self._build_with_temp_dir(['sdist', '--formats', 'gztar'],
'.tar.gz', sdist_directory,
config_settings)
orig, sdist.include_pyproject_toml = sdist.include_pyproject_toml, True
Copy link
Member

Choose a reason for hiding this comment

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

This monkeypatching style is not threadsafe. Overlapping calls could leave the original unset. I don't believe thread safety is needed here.

return self._build_with_temp_dir(['sdist', '--formats', 'gztar'],
'.tar.gz', sdist_directory,
config_settings)
orig, sdist.include_pyproject_toml = sdist.include_pyproject_toml, True
Copy link
Member

Choose a reason for hiding this comment

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

I'd love for this not to be monkey-patched, but assuming that's how it must be, this patch should be a one-line call to encapsulate the monkeypatch behavior and decouple it from the underlying call. Something like with self._enable_pyproject_toml():.

@jaraco
Copy link
Member

jaraco commented Dec 29, 2019

Given that this change will cause the pyproject.toml to be present in some sdists, how can we be confident that this change won't have the same regression as #1634?

Looking at this comment, the plan for setuptools is to start supporting PEP 517 builds of itself, which would in all likelihood cause the same regression. That comment specifically recommends for setuptools to exclude pyproject.toml from its source distribution, so probably all that's needed is to provide a specialized exclusion for Setuptools (but otherwise include the file for all other projects even on non-PEP 517 builds).

@uranusjr
Copy link
Member Author

Given that this change will cause the pyproject.toml to be present in some sdists, how can we be confident that this change won't have the same regression as #1634?

This patch is different from #1634 in that it only affects packages built through PEP 517 hooks, not setup.py sdist (because it does not go through the hooks; nothing in Setuptools does). So nobody using the usual build commands would be affected.

Looking at this comment, the plan for setuptools is to start supporting PEP 517 builds of itself, which would in all likelihood cause the same regression. That comment specifically recommends for setuptools to exclude pyproject.toml from its source distribution, so probably all that's needed is to provide a specialized exclusion for Setuptools (but otherwise include the file for all other projects even on non-PEP 517 builds).

I would be OK if Setuptools decide to include pyproject.toml by default even for non-PEP 517 build. Support for backend-path has been added to pip in pypa/pip#7394 and will be released in 21.0, but an exception would still be needed for Setuptools for quite a while, until people upgrades.

@jaraco
Copy link
Member

jaraco commented Dec 29, 2019

I think we want to go with the approach described in #1650, but I'd like to include your test, so I'll cherry-pick that.

@jaraco
Copy link
Member

jaraco commented Dec 29, 2019

In 8a7a627, I've cherry-picked your test onto #1650.

@jaraco jaraco closed this Dec 29, 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.

2 participants