-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
[internal] Replace pkgutil.get_data
with new read_resource
API
#16379
[internal] Replace pkgutil.get_data
with new read_resource
API
#16379
Conversation
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]
…nts distribution # 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]
pkgutil.get_data
with new read_resource
APIpkgutil.get_data
with new read_resource
API
Specifically re the |
@benjyw To the best of my knowledge, we can't add an pants/src/python/pants/conftest.py Lines 8 to 30 in ac24a35
|
On the contrary if I remember this correctly, having an explicit namespace package incantation in an |
src/python/pants/version.py
Outdated
# NB: We expect VERSION to always have an entry and want a runtime failure if this is false. | ||
pkgutil.get_data(__name__, "VERSION").decode().strip() # type: ignore[union-attr] | ||
read_resource("pants.bin", "VERSION").decode().strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the relevant bit of the description as a comment here would be helpful. Re: Benjy's command, could probably also reference #16359. But I don't think that you should change that here... should be tackled independently.
@benjyw Per Python docs, it seems like using a namespace package incantation would be a generally bad idea:
|
# 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]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
in particular, allowing read_resource(name, …), which is used commonly)
I don't think it matters being able to use __name__
. That seems more like a convenience. Notably, the dependency inference for assets added by @thejcannon works better when using the explicit package name.
src/python/pants/BUILD
Outdated
}, | ||
) | ||
|
||
python_test_utils(name="test_utils") | ||
|
||
python_distribution( | ||
name="pants-packaged", | ||
dependencies=["./__main__.py", ":resources"], | ||
dependencies=["./__main__.py", ":resources", ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad change
dependencies=["./__main__.py", ":resources", ], | |
dependencies=["./__main__.py", ":resources"], |
package = ".".join(chain((package_,), resource_parts[:-1])) | ||
resource = resource_parts[-1] | ||
|
||
return resources.read_binary(package, resource) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecated in 3.11: https://docs.python.org/3.11/library/importlib.resources.html#importlib.resources.read_binary
Now it's the much more awkward:
files(package).joinpath(resource).read_bytes()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. Note that a deprecation won't go away until Python 4.x, so we're safe to use it for a while, and the new API will not be available to us without backporting a new version of importlib-resources
.
The alternative here is vendoring in a new version of importlib-resources
, which doesn't have an issue with namespace packages.
I don't think this is a problem since we control the namespace. |
Generally, given the amount of dancing that we have to do to avoid an explicit |
Doesn't block this change though. |
Thanks for the feedback. I'm going to have to figure out how to make this change pass on 3.7, which I don't expect to get to for a few hours, but I imagine it's just going to be implementation details. |
I'm +1 on explicit namespace package too, if it's feasible. |
…RSION` resource is reliably available. # 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 @stuhood Tests were previously failing because To fix this, I've added a new package called I'm not 100% sure on the best place to put the pants/src/python/pants/_version/BUILD Lines 1 to 12 in 6582d44
|
(One possibility was creating a package called |
I think |
Internal changes: * [internal] Replace `pkgutil.get_data` with new `read_resource` API ([pantsbuild#16379](pantsbuild#16379)) * Bump bytes from 1.2.0 to 1.2.1 in /src/rust/engine ([pantsbuild#16389](pantsbuild#16389)) * Bump arc-swap from 1.5.0 to 1.5.1 in /src/rust/engine ([pantsbuild#16390](pantsbuild#16390)) * Bump console from 0.15.0 to 0.15.1 in /src/rust/engine ([pantsbuild#16392](pantsbuild#16392)) * Fix incorrect alpha sorting of a maintainer name ([pantsbuild#16401](pantsbuild#16401)) * Add Borja Lorente to Maintainers Emeritus ([pantsbuild#16382](pantsbuild#16382)) * Clarify limited logo usage rights granted by orgs ([pantsbuild#16384](pantsbuild#16384)) * Move Nora Howard to Maintainers Emeritus ([pantsbuild#16383](pantsbuild#16383)) * Include Chris Livingston in Maintainers Emeritus ([pantsbuild#16374](pantsbuild#16374)) * Bump clap from 3.2.14 to 3.2.15 in /src/rust/engine ([pantsbuild#16309](pantsbuild#16309)) * Bump crossbeam-channel from 0.5.5 to 0.5.6 in /src/rust/engine ([pantsbuild#16308](pantsbuild#16308)) * remove spurious quote mark ([pantsbuild#16361](pantsbuild#16361)) * A script to generate known versions data for the terraform binary ([pantsbuild#15958](pantsbuild#15958)) * Add more otel packages to default module mapping + fix to always use tuples ([pantsbuild#16345](pantsbuild#16345)) * [internal] upgrade `async-trait` crate ([pantsbuild#16347](pantsbuild#16347)) * Log dirtied nodes while backtracking. ([pantsbuild#16342](pantsbuild#16342)) * Added Opentelemetry to default Python module mapping ([pantsbuild#16337](pantsbuild#16337)) * Upgrade Toolchain Pants Plugin to 0.21.0 ([pantsbuild#16324](pantsbuild#16324)) * Bump bytes from 1.1.0 to 1.2.0 in /src/rust/engine ([pantsbuild#16245](pantsbuild#16245)) * Bump nix from 0.24.1 to 0.24.2 in /src/rust/engine ([pantsbuild#16307](pantsbuild#16307)) * Fix macos-10.15 brownout. ([pantsbuild#16317](pantsbuild#16317)) * Bump hyper from 0.14.19 to 0.14.20 in /src/rust/engine ([pantsbuild#16164](pantsbuild#16164)) * Bump clap from 3.2.8 to 3.2.14 in /src/rust/engine ([pantsbuild#16269](pantsbuild#16269)) * Don't break builds when pants.log upload fails ([pantsbuild#16294](pantsbuild#16294)) * [internal] Shell completion support ([pantsbuild#16200](pantsbuild#16200)) * Add debug output to help to differentiate retry cases ([pantsbuild#16277](pantsbuild#16277))
Internal changes: * [internal] Replace `pkgutil.get_data` with new `read_resource` API ([pantsbuild#16379](pantsbuild#16379)) * Bump bytes from 1.2.0 to 1.2.1 in /src/rust/engine ([pantsbuild#16389](pantsbuild#16389)) * Bump arc-swap from 1.5.0 to 1.5.1 in /src/rust/engine ([pantsbuild#16390](pantsbuild#16390)) * Bump console from 0.15.0 to 0.15.1 in /src/rust/engine ([pantsbuild#16392](pantsbuild#16392)) * Fix incorrect alpha sorting of a maintainer name ([pantsbuild#16401](pantsbuild#16401)) * Add Borja Lorente to Maintainers Emeritus ([pantsbuild#16382](pantsbuild#16382)) * Clarify limited logo usage rights granted by orgs ([pantsbuild#16384](pantsbuild#16384)) * Move Nora Howard to Maintainers Emeritus ([pantsbuild#16383](pantsbuild#16383)) * Include Chris Livingston in Maintainers Emeritus ([pantsbuild#16374](pantsbuild#16374)) * Bump clap from 3.2.14 to 3.2.15 in /src/rust/engine ([pantsbuild#16309](pantsbuild#16309)) * Bump crossbeam-channel from 0.5.5 to 0.5.6 in /src/rust/engine ([pantsbuild#16308](pantsbuild#16308)) * remove spurious quote mark ([pantsbuild#16361](pantsbuild#16361)) * A script to generate known versions data for the terraform binary ([pantsbuild#15958](pantsbuild#15958)) * Add more otel packages to default module mapping + fix to always use tuples ([pantsbuild#16345](pantsbuild#16345)) * [internal] upgrade `async-trait` crate ([pantsbuild#16347](pantsbuild#16347)) * Log dirtied nodes while backtracking. ([pantsbuild#16342](pantsbuild#16342)) * Added Opentelemetry to default Python module mapping ([pantsbuild#16337](pantsbuild#16337)) * Upgrade Toolchain Pants Plugin to 0.21.0 ([pantsbuild#16324](pantsbuild#16324)) * Bump bytes from 1.1.0 to 1.2.0 in /src/rust/engine ([pantsbuild#16245](pantsbuild#16245)) * Bump nix from 0.24.1 to 0.24.2 in /src/rust/engine ([pantsbuild#16307](pantsbuild#16307)) * Fix macos-10.15 brownout. ([pantsbuild#16317](pantsbuild#16317)) * Bump hyper from 0.14.19 to 0.14.20 in /src/rust/engine ([pantsbuild#16164](pantsbuild#16164)) * Bump clap from 3.2.8 to 3.2.14 in /src/rust/engine ([pantsbuild#16269](pantsbuild#16269)) * Don't break builds when pants.log upload fails ([pantsbuild#16294](pantsbuild#16294)) * [internal] Shell completion support ([pantsbuild#16200](pantsbuild#16200)) * Add debug output to help to differentiate retry cases ([pantsbuild#16277](pantsbuild#16277))
Internal changes: * [internal] Replace `pkgutil.get_data` with new `read_resource` API ([pantsbuild#16379](pantsbuild#16379)) * Bump bytes from 1.2.0 to 1.2.1 in /src/rust/engine ([pantsbuild#16389](pantsbuild#16389)) * Bump arc-swap from 1.5.0 to 1.5.1 in /src/rust/engine ([pantsbuild#16390](pantsbuild#16390)) * Bump console from 0.15.0 to 0.15.1 in /src/rust/engine ([pantsbuild#16392](pantsbuild#16392)) * Fix incorrect alpha sorting of a maintainer name ([pantsbuild#16401](pantsbuild#16401)) * Add Borja Lorente to Maintainers Emeritus ([pantsbuild#16382](pantsbuild#16382)) * Clarify limited logo usage rights granted by orgs ([pantsbuild#16384](pantsbuild#16384)) * Move Nora Howard to Maintainers Emeritus ([pantsbuild#16383](pantsbuild#16383)) * Include Chris Livingston in Maintainers Emeritus ([pantsbuild#16374](pantsbuild#16374)) * Bump clap from 3.2.14 to 3.2.15 in /src/rust/engine ([pantsbuild#16309](pantsbuild#16309)) * Bump crossbeam-channel from 0.5.5 to 0.5.6 in /src/rust/engine ([pantsbuild#16308](pantsbuild#16308)) * remove spurious quote mark ([pantsbuild#16361](pantsbuild#16361)) * A script to generate known versions data for the terraform binary ([pantsbuild#15958](pantsbuild#15958)) * Add more otel packages to default module mapping + fix to always use tuples ([pantsbuild#16345](pantsbuild#16345)) * [internal] upgrade `async-trait` crate ([pantsbuild#16347](pantsbuild#16347)) * Log dirtied nodes while backtracking. ([pantsbuild#16342](pantsbuild#16342)) * Added Opentelemetry to default Python module mapping ([pantsbuild#16337](pantsbuild#16337)) * Upgrade Toolchain Pants Plugin to 0.21.0 ([pantsbuild#16324](pantsbuild#16324)) * Bump bytes from 1.1.0 to 1.2.0 in /src/rust/engine ([pantsbuild#16245](pantsbuild#16245)) * Bump nix from 0.24.1 to 0.24.2 in /src/rust/engine ([pantsbuild#16307](pantsbuild#16307)) * Fix macos-10.15 brownout. ([pantsbuild#16317](pantsbuild#16317)) * Bump hyper from 0.14.19 to 0.14.20 in /src/rust/engine ([pantsbuild#16164](pantsbuild#16164)) * Bump clap from 3.2.8 to 3.2.14 in /src/rust/engine ([pantsbuild#16269](pantsbuild#16269)) * Don't break builds when pants.log upload fails ([pantsbuild#16294](pantsbuild#16294)) * [internal] Shell completion support ([pantsbuild#16200](pantsbuild#16200)) * Add debug output to help to differentiate retry cases ([pantsbuild#16277](pantsbuild#16277)) * Fix process_executor ([pantsbuild#16428](pantsbuild#16428))
Internal changes: * [internal] Replace `pkgutil.get_data` with new `read_resource` API ([#16379](#16379)) * Bump bytes from 1.2.0 to 1.2.1 in /src/rust/engine ([#16389](#16389)) * Bump arc-swap from 1.5.0 to 1.5.1 in /src/rust/engine ([#16390](#16390)) * Bump console from 0.15.0 to 0.15.1 in /src/rust/engine ([#16392](#16392)) * Fix incorrect alpha sorting of a maintainer name ([#16401](#16401)) * Add Borja Lorente to Maintainers Emeritus ([#16382](#16382)) * Clarify limited logo usage rights granted by orgs ([#16384](#16384)) * Move Nora Howard to Maintainers Emeritus ([#16383](#16383)) * Include Chris Livingston in Maintainers Emeritus ([#16374](#16374)) * Bump clap from 3.2.14 to 3.2.15 in /src/rust/engine ([#16309](#16309)) * Bump crossbeam-channel from 0.5.5 to 0.5.6 in /src/rust/engine ([#16308](#16308)) * remove spurious quote mark ([#16361](#16361)) * A script to generate known versions data for the terraform binary ([#15958](#15958)) * Add more otel packages to default module mapping + fix to always use tuples ([#16345](#16345)) * [internal] upgrade `async-trait` crate ([#16347](#16347)) * Log dirtied nodes while backtracking. ([#16342](#16342)) * Added Opentelemetry to default Python module mapping ([#16337](#16337)) * Upgrade Toolchain Pants Plugin to 0.21.0 ([#16324](#16324)) * Bump bytes from 1.1.0 to 1.2.0 in /src/rust/engine ([#16245](#16245)) * Bump nix from 0.24.1 to 0.24.2 in /src/rust/engine ([#16307](#16307)) * Fix macos-10.15 brownout. ([#16317](#16317)) * Bump hyper from 0.14.19 to 0.14.20 in /src/rust/engine ([#16164](#16164)) * Bump clap from 3.2.8 to 3.2.14 in /src/rust/engine ([#16269](#16269)) * Don't break builds when pants.log upload fails ([#16294](#16294)) * [internal] Shell completion support ([#16200](#16200)) * Add debug output to help to differentiate retry cases ([#16277](#16277)) * Fix process_executor ([#16428](#16428))
Unfortunately this has broken the release process. In the installed pantsbuild.pants wheel _version/VERSION is not a symlink, during wheel creation it turns into a real file that is a copy of the original one. And this happens after we do the version rewrite ( pants/build-support/bin/_release_helper.py Line 639 in defc952
|
…` API (pantsbuild#16379)" This reverts commit a01f375.
Granted, the version rewrite stuff is hacky AF |
…` API (pantsbuild#16379)" This reverts commit a01f375. [ci skip-rust] [ci skip-build-wheels]
Features: * Revert "Replace `pkgutil.get_data` with new `read_resource` API (pantsbuild#16379)" (pantsbuild#16433) * Upgrade Pex to 2.1.103. (pantsbuild#16426)
…PI (pantsbuild#16379)" (pantsbuild#16433)" This reverts commit 3f16596.
…PI (pantsbuild#16379)" (pantsbuild#16433)" This reverts commit 3f16596. (cherry picked from commit 17ecac0) # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
…antsbuild#16379) Pants uses `pkgutil.get_data()` in all manner of places to load resource files that are included within the Pants Python package. `get_data()` is not supported in the PEP302 pluggable importer used by PyOxidizer. `importlib.resources`, however, _is_ supported. `importlib.resources` (as far as PyOxidizer is concerned, at the very least) has some caveats: * Resources can only be loaded from packages that contain an `__init__.py` file (i.e. are a non-namespace package) * Resources cannot be `.py` files This adds a shim function called `read_resource()` that allows reading resource files with more-or-less the same API as `pkgutil.get_data()` (in particular, allowing `read_resource(__name__, …)`, which is used commonly), but plugs into `importlib.resources.read_binary()` to allow for better portability. A symlink to `src/python/pants/VERSION` is added at `src/python/pants/_version/VERSION`, so that the resource can be loaded with the new API, without immediately breaking the hard-coded version file location required by our setup script. Finally, the python dependency parser has been renamed to `dependency_parser_py`, so that it can be loaded as a resource, and still treated as a `python_source` for Pants linting purposes. Addresses pantsbuild#7369.
Internal changes: * [internal] Replace `pkgutil.get_data` with new `read_resource` API ([pantsbuild#16379](pantsbuild#16379)) * Bump bytes from 1.2.0 to 1.2.1 in /src/rust/engine ([pantsbuild#16389](pantsbuild#16389)) * Bump arc-swap from 1.5.0 to 1.5.1 in /src/rust/engine ([pantsbuild#16390](pantsbuild#16390)) * Bump console from 0.15.0 to 0.15.1 in /src/rust/engine ([pantsbuild#16392](pantsbuild#16392)) * Fix incorrect alpha sorting of a maintainer name ([pantsbuild#16401](pantsbuild#16401)) * Add Borja Lorente to Maintainers Emeritus ([pantsbuild#16382](pantsbuild#16382)) * Clarify limited logo usage rights granted by orgs ([pantsbuild#16384](pantsbuild#16384)) * Move Nora Howard to Maintainers Emeritus ([pantsbuild#16383](pantsbuild#16383)) * Include Chris Livingston in Maintainers Emeritus ([pantsbuild#16374](pantsbuild#16374)) * Bump clap from 3.2.14 to 3.2.15 in /src/rust/engine ([pantsbuild#16309](pantsbuild#16309)) * Bump crossbeam-channel from 0.5.5 to 0.5.6 in /src/rust/engine ([pantsbuild#16308](pantsbuild#16308)) * remove spurious quote mark ([pantsbuild#16361](pantsbuild#16361)) * A script to generate known versions data for the terraform binary ([pantsbuild#15958](pantsbuild#15958)) * Add more otel packages to default module mapping + fix to always use tuples ([pantsbuild#16345](pantsbuild#16345)) * [internal] upgrade `async-trait` crate ([pantsbuild#16347](pantsbuild#16347)) * Log dirtied nodes while backtracking. ([pantsbuild#16342](pantsbuild#16342)) * Added Opentelemetry to default Python module mapping ([pantsbuild#16337](pantsbuild#16337)) * Upgrade Toolchain Pants Plugin to 0.21.0 ([pantsbuild#16324](pantsbuild#16324)) * Bump bytes from 1.1.0 to 1.2.0 in /src/rust/engine ([pantsbuild#16245](pantsbuild#16245)) * Bump nix from 0.24.1 to 0.24.2 in /src/rust/engine ([pantsbuild#16307](pantsbuild#16307)) * Fix macos-10.15 brownout. ([pantsbuild#16317](pantsbuild#16317)) * Bump hyper from 0.14.19 to 0.14.20 in /src/rust/engine ([pantsbuild#16164](pantsbuild#16164)) * Bump clap from 3.2.8 to 3.2.14 in /src/rust/engine ([pantsbuild#16269](pantsbuild#16269)) * Don't break builds when pants.log upload fails ([pantsbuild#16294](pantsbuild#16294)) * [internal] Shell completion support ([pantsbuild#16200](pantsbuild#16200)) * Add debug output to help to differentiate retry cases ([pantsbuild#16277](pantsbuild#16277)) * Fix process_executor ([pantsbuild#16428](pantsbuild#16428))
…sbuild#16379)" (pantsbuild#16433) This reverts commit a01f375. [ci skip-rust] [ci skip-build-wheels]
Features: * Revert "Replace `pkgutil.get_data` with new `read_resource` API (pantsbuild#16379)" (pantsbuild#16433) * Upgrade Pex to 2.1.103. (pantsbuild#16426)
Pants uses
pkgutil.get_data()
in all manner of places to load resource files that are included within the Pants Python package.get_data()
is not supported in the PEP302 pluggable importer used by PyOxidizer.importlib.resources
, however, is supported.importlib.resources
(as far as PyOxidizer is concerned, at the very least) has some caveats:__init__.py
file (i.e. are a non-namespace package).py
filesThis adds a shim function called
read_resource()
that allows reading resource files with more-or-less the same API aspkgutil.get_data()
(in particular, allowingread_resource(__name__, …)
, which is used commonly), but plugs intoimportlib.resources.read_binary()
to allow for better portability.A symlink to
src/python/pants/VERSION
is added atsrc/python/pants/_version
, so that the resource can be loaded with the new API, without immediately breaking the hard-coded version file location required by our setup script.Finally, the python dependency parser has been renamed to
dependency_parser_py
, so that it can be loaded as a resource, and still treated as apython_source
for Pants linting purposes.Addresses #7369.