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

feat: add interpreter_version_info to py_runtime #1671

Merged
merged 4 commits into from
Jan 13, 2024
Merged

Conversation

mattem
Copy link
Collaborator

@mattem mattem commented Jan 7, 2024

Adds an interpreter_version_info attribute to the py_runtime and associated provider that maps to the sys.version_info values. This allows the version of the interpreter to be known statically, which can be useful for rule sets that depend on the interpreter, and need to build environments / pathing that contain version info (virtualenvs for example).

@mattem mattem force-pushed the interpreter_version branch 4 times, most recently from 245f06c to 238de8e Compare January 7, 2024 15:52
Copy link
Collaborator

@f0rmiga f0rmiga left a comment

Choose a reason for hiding this comment

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

I'll let other folks bikeshed if they want.

python/private/common/py_runtime_rule.bzl Outdated Show resolved Hide resolved
Copy link
Contributor

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

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

My main comment is to use a dict/struct instead of list. This makes it easier to know which parts are check for the relevant pieces. Plus we can stash extra implementation-specific parts in there if necessary.

i.e. accept a attr.string_dict on the rule and and convert it to a struct() for the provider. Convert particular keys to their appropriate types (e.g. major/minor/patch to ints, similar to sys.version_info)

Please add tests. You can add them to //tests/py_runtime

python/private/common/providers.bzl Show resolved Hide resolved
@mattem
Copy link
Collaborator Author

mattem commented Jan 9, 2024

@rickeylev Can I have another pass on this please?

@rickeylev
Copy link
Contributor

Looks pretty good! Thank you for adding tests.

Can you please add an entry to CHANGELOG.md? Then it's ready to merge.

@rickeylev rickeylev added this pull request to the merge queue Jan 13, 2024
Merged via the queue into main with commit 5238141 Jan 13, 2024
7 checks passed
@rickeylev rickeylev deleted the interpreter_version branch January 17, 2024 17:12
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.

3 participants