Fix the testutil wheel's imports not working due to namespace packages
#10725
Conversation
[ci skip-rust] [ci skip-build-wheels]
| @@ -116,7 +116,7 @@ function pkg_testutil_install_test() { | |||
| shift | |||
| local PIP_ARGS=("$@") | |||
| pip install "${PIP_ARGS[@]}" "pantsbuild.pants.testutil==${version}" && \ | |||
| python -c "import pants.testutil" | |||
| python -c "import pants.testutil.option_util" | |||
There was a problem hiding this comment.
This doesn't actually trigger an error on the master branch, for some reason. It imports just fine. I'm not sure why or how to reproduce in release.sh, even though I reproduce everywhere else like running pex testutil==2.0.0a1 then import pants.testutil.option_util.
| @@ -25,7 +25,6 @@ python_distribution( | |||
| provides=setup_py( | |||
| name='pantsbuild.pants', | |||
| description='A scalable build tool for large, complex, heterogeneous repos.', | |||
| namespace_packages=['pants', 'pants.backend'], | |||
There was a problem hiding this comment.
I'm not sure why we were declaring pants.backend. That is inferred due to pants, afaict.
| @@ -0,0 +1 @@ | |||
| __import__("pkg_resources").declare_namespace(__name__) | |||
There was a problem hiding this comment.
I considered also adding this to pants/testutil/__init__.py, but I don't think it's necessary. We didn't have that declaration before, and testutil worked. I think we only need it at the top level.
There was a problem hiding this comment.
We only need it in packages that are shared across multiple dists/source roots, so yeah.
There was a problem hiding this comment.
Might be worth adding a comment that this is necessary because we build two wheels under this source root. Otherwise a naive reader might wonder why it's necessary to have this when pants is defined only under this single source root.
| for arg in {"data_files", "package_dir", "package_data", "packages"}: | ||
| for arg in { | ||
| "data_files", | ||
| "namespace_packages", | ||
| "package_dir", | ||
| "package_data", | ||
| "packages", | ||
| "install_requires", | ||
| }: |
There was a problem hiding this comment.
Alternatively, we could allow users to set some of these and have us extend the values, rather than overwriting like we were doing.
I think I err on the side of banning, though. As a user, if I set namespace_packages=['foo'], I think I would be confused if Pants ended up adding two additional values to it. It's not clear that Pants auto-magically sets these 6 values, but it doesn't touch any other kwargs.
If we get user feedback that they really need to set these, we can add a safety valve. (Actually, that safety valve already exists. They can use a custom SetupKwargs implementation and in the constructor to SetupKwargs, set the arg _allow_banned_keys=True.
There was a problem hiding this comment.
Would that work, since we overwrite? Wouldn't we have to check if the value exists and only write it if not?
There was a problem hiding this comment.
Would that work, since we overwrite? Wouldn't we have to check if the value exists and only write it if not?
Ah, true. I can fix that, though, if you think it's a good idea?
That would mean that by default, you can't set those values because it would be too confusing. But if you really want a safety valve, you have it.
There was a problem hiding this comment.
Yes, exactly. With a comment explaining that :)
This ensures that things stay stable across Python versions. Python 3.8 added Pickle 5, but we can't use that for Python 3.6 and 3.7 # 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]
* Add comment about check_inits.py * Fix the precommit hook * Fix setup_py.py to not override those 6 values, and explain why we do that # 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]
Imports stopped working for
testutilin 2.0.0.dev2 because we removed the line__import__("pkg_resources").declare_namespace(__name__)fromsrc/python/pants/__init__.py, so we were no longer using namespace packages and we had two dists with the same namespace.This PR also fixes the
setup-pygoal to error if users are manually settingnamespace_packagesandinstall_requires. Pants manages that for users. Before, we were silently overwriting values they set, which was confusing.[ci skip-rust]
[ci skip-build-wheels]