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

Add tool to help with "bootstraping" build #4389

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

abravalheri
Copy link
Contributor

@abravalheri abravalheri commented May 23, 2024

Summary of changes

Now that wheel is vendored, setuptools can be bootstrapped in a relatively easy way,
without the need of python setup.py install/develop/....

This PR adds a small script that follows PEP 517 (or at least a stripped down version of it, super-specialised for setuptools) + general instructions to the "Development Guide".

Closes #2828
Closes #2532 ? (pip install -e . already works fine for devs that have access to pip, so the only missing piece was bootstrapping)

Pull Request Checklist

@@ -167,6 +167,7 @@ exclude = [
"newsfragments*",
"docs",
"docs.*",
"build*",
Copy link
Contributor Author

@abravalheri abravalheri May 23, 2024

Choose a reason for hiding this comment

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

When testing localy, I found pesky build/lib/**/*.py files being included in the wheel if the build directory was not empty.

This is also the reason why I changed MANIFEST.in.

Copy link
Member

Choose a reason for hiding this comment

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

Ugh. That doesn't seem right... adding a concern that seemingly affects all builds for something that's only affected when a bootstrap was run. Perhaps instead the bootstrap routine could remove the build directory after running?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would make more sense if the routine could remove the build directory before running...

I noted that if build exists find_packages will include it. Once it is listed in packages, changing MANIFEST.in does not seem to make an effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it make more sense if instead we do:

[tool.setuptools.packages.find]
include = [
  "setuptools*",
  "pkg_resources*",
  "_distutils_hack*",
]
exclude = [
	"*.tests",
	"*.tests.*",
]

I think this simplifies things a bit and it is optimised for the case when the probability of adding .py in places outside the intended package directories is more likely than adding new package directories.

If the probabilities are reversed, then it makes less sense.

Copy link
Contributor Author

@abravalheri abravalheri May 24, 2024

Choose a reason for hiding this comment

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

Ugh. That doesn't seem right... adding a concern that seemingly affects all builds for something that's only affected when a bootstrap was run. Perhaps instead the bootstrap routine could remove the build directory after running?

Probably the reason why python -m build works is because build copies the code to a temporary directory? (I guess)
But that is not a PEP 517 requirement...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a reproducer for the observed problem in #4394.

(other than pip install . or pip wheel ., running the test suite with tox itself also produces the build directory, so if anyone tries to run tox before pip install ., they also end-up with spurious files).

@@ -2680,7 +2680,7 @@ def parse_map(
_data = data.items()
else:
_data = split_sections(data)
maps: Dict[str, Dict[str, "EntryPoint"]] = {}
maps: Dict[str, Dict[str, EntryPoint]] = {}
Copy link
Contributor Author

@abravalheri abravalheri May 23, 2024

Choose a reason for hiding this comment

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

For some reason the linting tools started to complain.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we fix this in a separate PR or commit, since it's unrelated to the proposal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I will work on it (this PR was more of a proof of concept).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abravalheri
Copy link
Contributor Author

Hi @jaraco, do think we should pursue something like this for setuptools?

@abravalheri abravalheri marked this pull request as ready for review May 23, 2024 18:51
@abravalheri abravalheri requested a review from jaraco May 23, 2024 19:37
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.

LGTM. It's a little annoying that this change introduces the 'build' and 'dist' directory concerns. Maybe those can be dealt with separately.

I do wonder if Setuptools should be in the business of providing the bootstrapping tool. IMO, if a downstream packager can't use pip, they should write their own PEP 517 builder/installer (that is, they should maintain it and not setuptools).

@FFY00 How do you feel about the approach? Should Setuptools provide this or could it be maintained outside setuptools?

@@ -2680,7 +2680,7 @@ def parse_map(
_data = data.items()
else:
_data = split_sections(data)
maps: Dict[str, Dict[str, "EntryPoint"]] = {}
maps: Dict[str, Dict[str, EntryPoint]] = {}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we fix this in a separate PR or commit, since it's unrelated to the proposal?

@@ -167,6 +167,7 @@ exclude = [
"newsfragments*",
"docs",
"docs.*",
"build*",
Copy link
Member

Choose a reason for hiding this comment

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

Ugh. That doesn't seem right... adding a concern that seemingly affects all builds for something that's only affected when a bootstrap was run. Perhaps instead the bootstrap routine could remove the build directory after running?

MANIFEST.in Outdated
Comment on lines 21 to 22
prune dist
prune build
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised this behavior isn't the default. Is it the case that if a dist directory existed prior to this change, it would end up in the sdist? That seems like bad default behavior and would have come up in other projects. I'd like to avoid this concern if possible. It's another case where adopting setuptools_scm will also bypass the concern.

Copy link
Contributor Author

@abravalheri abravalheri May 24, 2024

Choose a reason for hiding this comment

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

We probably don't need this one. The problem is likely to be with tool.setuptools.packages.find: #4394.

@jaraco jaraco requested a review from FFY00 May 24, 2024 10:55
Copy link
Contributor Author

@abravalheri abravalheri left a comment

Choose a reason for hiding this comment

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

@jaraco, thank you very much for the review.

I suppose another alternative is to document something like the following:

$ git clone https://github.com/pypa/setuptools
$ cd setuptools
$ python3 -c 'from setuptools.build_meta import build_wheel; build_wheel("./dist")'
$ python3 -m zipfile -e ./dist/setuptools-*.whl $TARGET_DIR

(No custom script required)

The disadvantage of this approach is that building the wheel directly from the source tree may yield different results than the wheel that we upload to PyPI (building the wheel from the sdist is usually more consistent).

@@ -2680,7 +2680,7 @@ def parse_map(
_data = data.items()
else:
_data = split_sections(data)
maps: Dict[str, Dict[str, "EntryPoint"]] = {}
maps: Dict[str, Dict[str, EntryPoint]] = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I will work on it (this PR was more of a proof of concept).

@@ -167,6 +167,7 @@ exclude = [
"newsfragments*",
"docs",
"docs.*",
"build*",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would make more sense if the routine could remove the build directory before running...

I noted that if build exists find_packages will include it. Once it is listed in packages, changing MANIFEST.in does not seem to make an effect.

MANIFEST.in Outdated
Comment on lines 21 to 22
prune dist
prune build
Copy link
Contributor Author

@abravalheri abravalheri May 24, 2024

Choose a reason for hiding this comment

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

We probably don't need this one. The problem is likely to be with tool.setuptools.packages.find: #4394.

@FFY00
Copy link
Member

FFY00 commented May 28, 2024

I do wonder if Setuptools should be in the business of providing the bootstrapping tool. IMO, if a downstream packager can't use pip, they should write their own PEP 517 builder/installer (that is, they should maintain it and not setuptools).

@FFY00 How do you feel about the approach? Should Setuptools provide this or could it be maintained outside setuptools?

I don't have any complains about this, but I'd note the reason why downstream packagers usually can't use pip is because it vendors dependencies, and devendoring them for bootstrap is a mess. In Arch Linux, for example, we would similarly want to devendor the wheel dependency.

That said, this script, however, would be helpful regardless.

@nanonyme
Copy link

I do wonder if Setuptools should be in the business of providing the bootstrapping tool. IMO, if a downstream packager can't use pip, they should write their own PEP 517 builder/installer (that is, they should maintain it and not setuptools).
@FFY00 How do you feel about the approach? Should Setuptools provide this or could it be maintained outside setuptools?

I don't have any complains about this, but I'd note the reason why downstream packagers usually can't use pip is because it vendors dependencies, and devendoring them for bootstrap is a mess. In Arch Linux, for example, we would similarly want to devendor the wheel dependency.

That said, this script, however, would be helpful regardless.

There's no reason to do this. There is a clear bootstrapping path starting from flit_core which provides you fully working pypa/build and pypa/installer which you can then use to process pypa/wheel and finally pypa/setuptools and pypa/pip. Setuptools is not the beginning of PEP517 bootstrap chain and does not need to implement special tools. It does, however, need to support building itself with in-tree backend which is currently broken with newest pyproject-hooks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants