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

Fix __package_version__ attr literal evaluation. #322

Merged
merged 2 commits into from
Feb 3, 2025

Conversation

jameshilliard
Copy link
Contributor

Dynamic metadata should be compatible with ast.literal_eval() so that import fallback is not needed. It's important to avoid the import fallback as that pulls in a number of unnecessary build dependencies.

See:
https://setuptools.pypa.io/en/latest/userguide/pyproject_config.html#dynamic-metadata

@airtower-luna
Copy link

To explain the practical importance of this: Having to import the module simply to read metadata causes trouble for cross-builds (this patch is the result of a discussion on the Buildroot mailing list), because suddenly all target runtime dependencies are needed in the host environment, too, only to be able to read the version.

@vfazio
Copy link

vfazio commented Jan 27, 2025

Thanks for doing this James.

I highlighted a few other things we should maybe change too in https://lore.kernel.org/buildroot/CADvTj4psKDjXCC3691x-eboKJ=aLVXKw9g7pRASOvpRq_xYz4w@mail.gmail.com/T/#mb7202713e922c31c328e3539a4a5ce606aabfb44

  • bump the setuptools version requirement to 62.6.0 to avoid build failures on earlier versions
  • drop the pyasyncore and cryptography from the build-system dependencies after fixing this to be a literal

@jameshilliard
Copy link
Contributor Author

  • bump the setuptools version requirement to 62.6.0 to avoid build failures on earlier versions

  • drop the pyasyncore and cryptography from the build-system dependencies after fixing this to be a literal

Added these changes as well.

@simonrob
Copy link
Owner

Thanks for the PR. The current approach was arrived at after a surprisingly long process. I'm not necessarily averse to making this change, but would want to make sure it doesn't just cause the same issue the other way around. After all, __version__ is more or less standard, whereas __package_version__ is just for this script, and only used when building. For example, I have no insight into whether the __version__ value is currently being parsed and used downstream.

Of course, all of this stems from my early (and in hindsight probably ill-advised) choice of ISO 8601 date rather than a more pythonic version specifier. An easier change could just be to switch __version__ to the . rather than the - format. This would simplify building and remove the need for __package_version__ entirely. I'll think about whether this would have any other impacts.

Other notes:

  • ISO 8601 dates have leading zeros in days/months < 10, whereas the date format specified in PEP 440 does not. This is why the current approach is more complex than, e.g., __version__.replace('-', '.').
  • Re: the comment about ast.literal_eval() – both ast.literal_eval("2024-11-13") and ast.literal_eval("2024.11.13") fail, though for different reasons. Presumably this is not an issue?

@airtower-luna
Copy link

Re: the comment about ast.literal_eval() – both ast.literal_eval("2024-11-13") and ast.literal_eval("2024.11.13") fail, though for different reasons. Presumably this is not an issue?

Both are missing quotes that'd be in the code. Try ast.literal_eval('"2024.11.13"'). 🙂

Dynamic metadata should be compatible with ast.literal_eval() so
that import fallback is not needed. It's important to avoid the
import fallback as that pulls in a number of unnecessary build
dependencies.

See:
https://setuptools.pypa.io/en/latest/userguide/pyproject_config.html#dynamic-metadata
@jameshilliard
Copy link
Contributor Author

  • ISO 8601 dates have leading zeros in days/months < 10, whereas the date format specified in PEP 440 does not. This is why the current approach is more complex than, e.g., __version__.replace('-', '.').

Should be fixed now.

I'm not necessarily averse to making this change, but would want to make sure it doesn't just cause the same issue the other way around. After all, __version__ is more or less standard, whereas __package_version__ is just for this script, and only used when building. For example, I have no insight into whether the __version__ value is currently being parsed and used downstream.

Usually only build tooling like setuptools need to do ast parsing to avoid imports so anything accessing __version__ through imports at runtime should be fine.

@simonrob
Copy link
Owner

simonrob commented Feb 3, 2025

Thanks for the additional detail (and 🤦 re: literal_eval()...)

I can't find any downstream projects that make any use of __version__ that would be affected, so I'll go ahead and merge this. The pyproject.toml import hack always bugged me, so it's great this refines that too. Thanks for the contribution!

@simonrob simonrob merged commit 02770ad into simonrob:main Feb 3, 2025
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.

4 participants