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

Simplify the setup-py rules. #10529

Merged
merged 3 commits into from Aug 2, 2020
Merged

Conversation

benjyw
Copy link
Sponsor Contributor

@benjyw benjyw commented Aug 1, 2020

The old code repeated some logic now centralized in python_sources.py and
elsewhere, notably the ancestor __init__.py discovery. We now just use that logic
instead of recapitulating it.

This required some changes to the helper function that discovers packages
and package data (for the purpose of writing those into the setup.py metadata).
That code is now simpler, and in particular no longer needs to know anything
about targets. This leads to a very slight behavior change: previously we would
infer the existence of a "python package" from the existence of resources, even when
no .py files were available. This is incorrect: to load a resources using pkgutil or
pkg_resources, they must be loaded relative to some actual Python package
(which by definition requires the presence of a .py file, __init__.py or other).
Now we handle this correctly - a nice case where the simpler code is also more correct.

This change also removes the check that injected missing __init__.py files must be
empty. This check was insufficient, e.g., an __init__.py containing a namespace
package invocation should also be fine to inject. And for setup.py this often must
be the case - if you have multiple dists in the same source tree, they must be in
a namespace package or the dists won't import correctly. Instead we just blindly
inject any ancestor __init__.py, and rely on the user that their __init__.py
hygiene is good. In particular, if an __init__.py has deps, injection won't do
anything special with those, so they will have to actually depend on the __init__.py,
either explicitly or via inference. In the setup-py case, however, ancestor
__init__.py files are almost certainly just to create namespace packages, as
any code in them would be embedded in multiple dists, which is a bad idea.

Since that empty file check is removed, the remaining code was too trivial to
exist on its own, so it was removed, and its one-line implementation moved to
the two call sites.

[ci skip-rust]

[ci skip-build-wheels]

[ci skip-rust]

[ci skip-build-wheels]
@coveralls
Copy link

coveralls commented Aug 1, 2020

Coverage Status

Coverage remained the same at 0.0% when pulling 5096356 on benjyw:setup_py_simplification into 685e51d on pantsbuild:master.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Yay simplification!

I agree with the decision to no longer error if the __init__.py file has content in it.

src/python/pants/backend/python/rules/run_setup_py.py Outdated Show resolved Hide resolved
@@ -161,7 +157,7 @@ def test_generate_chroot(self) -> None:
"name": "foo",
"version": "1.2.3",
"package_dir": {"": "src"},
"packages": ["foo", "foo.qux", "foo.resources.js"],
Copy link
Contributor

Choose a reason for hiding this comment

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

This was a bug, right?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I think so (see description of this PR).

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@benjyw
Copy link
Sponsor Contributor Author

benjyw commented Aug 2, 2020

Added a test for files(), which actually caught an issue with them. Fixed now, and will update the online docs appropriately once this is released.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@benjyw benjyw merged commit 41ec94b into pantsbuild:master Aug 2, 2020
@benjyw benjyw deleted the setup_py_simplification branch August 2, 2020 06:31
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.

None yet

3 participants