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(config_settings): add extra settings for toolchain platforms #1743

Closed
wants to merge 13 commits into from

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented Feb 5, 2024

Prior to this PR //python/config_settings would contain only the
config_setting targets for the Python versions from //python:versions.bzl.
We would only match the python_version configuration and if someone needed
matching against the constraint_values they would have to redefine the
config_setting values themselves as match_all helper from bazel skylib
cannot be used in this case do to how specialization priority of
config_setting in a select works - match_all would create select
statement entries that are not specialized enough, i.e. they would not have the
python version config setting and the target platform constraint values in a single
config_setting.

This PR creates additional config_setting values that are useful for rulesets
implementing handling of platform-specific wheels in a multi-python version scenario
and it lays a good foundation for supporting download_only = True in a better way
within the pip.parse extension itself.

NOTE: config_setting values are only created for the Python toolchain target
platforms.

@aignas
Copy link
Collaborator Author

aignas commented Feb 12, 2024

Thinking about it a little more, I am wondering if it is OK to have this code in here. It may be better to not expose extra config settings. If the os/arch-specific selects are needed, maybe they should be defined/passed in the pip.parse instead. This is so that the rules_python API surface is kept to a minimum and so that we don't make things too coupled.

That said, having the mapping between a python version string and the expected flag values that are passed to match_any would be useful, because that is a property of the python_version string flag.

Feel free to comment if you disagree with my statement above.

@aignas aignas closed this Feb 12, 2024
aignas added a commit that referenced this pull request Feb 12, 2024
The PR #1743 explored the idea of creating extra config settings for
each target platform that our toolchain is targetting, however that has
a drawback of not being usable in `bzlmod` if someone built Python for
a platform that we don't provide a toolchain for and tried to use the
`pip.parse` machinery with that by providing the
`python_interpreter_target`. That is a niche usecase, but `rules_python`
is a core ruleset that should only provide abstractions/helpers that
work in all cases or make it possible to extend things.

This explores a way to decouple the definition of the available
`config_settings` values and how they are constructed by adding an extra
`is_python_config_setting` macro, that could be used to declare the
config settings from within the `pip.parse` hub repo. This makes the
work in #1744 to support whl-only hub repos more self-contained.

Supersedes #1743.
aignas added a commit that referenced this pull request Feb 12, 2024
aignas added a commit that referenced this pull request Feb 12, 2024
The PR #1743 explored the idea of creating extra config settings for
each target platform that our toolchain is targetting, however that has
a drawback of not being usable in `bzlmod` if someone built Python for
a platform that we don't provide a toolchain for and tried to use the
`pip.parse` machinery with that by providing the
`python_interpreter_target`. That is a niche usecase, but `rules_python`
is a core ruleset that should only provide abstractions/helpers that
work in all cases or make it possible to extend things.

This explores a way to decouple the definition of the available
`config_settings` values and how they are constructed by adding an extra
`is_python_config_setting` macro, that could be used to declare the
config settings from within the `pip.parse` hub repo. This makes the
work in #1744 to support whl-only hub repos more self-contained.

Supersedes #1743.
aignas added a commit to aignas/rules_python that referenced this pull request Mar 16, 2024
The PR bazelbuild#1743 explored the idea of creating extra config settings for
each target platform that our toolchain is targetting, however that has
a drawback of not being usable in `bzlmod` if someone built Python for
a platform that we don't provide a toolchain for and tried to use the
`pip.parse` machinery with that by providing the
`python_interpreter_target`. That is a niche usecase, but `rules_python`
is a core ruleset that should only provide abstractions/helpers that
work in all cases or make it possible to extend things.

This explores a way to decouple the definition of the available
`config_settings` values and how they are constructed by adding an extra
`is_python_config_setting` macro, that could be used to declare the
config settings from within the `pip.parse` hub repo. This makes the
work in bazelbuild#1744 to support whl-only hub repos more self-contained.

Supersedes bazelbuild#1743.
github-merge-queue bot pushed a commit that referenced this pull request Mar 18, 2024
The PR #1743 explored the idea of creating extra config settings for
each target platform that our toolchain is targetting, however that has
a drawback of not being usable in `bzlmod` if someone built Python for
a platform that we don't provide a toolchain for and tried to use the
`pip.parse` machinery with that by providing the
`python_interpreter_target`. That is a niche usecase, but `rules_python`
is a core ruleset that should only provide abstractions/helpers that
work in all cases or make it possible to extend things.

This explores a way to decouple the definition of the available
`config_settings` values and how they are constructed by adding an extra
`is_python_config_setting` macro, that could be used to declare the
config settings from within the `pip.parse` hub repo. This makes the
work in #1744 to support whl-only hub repos more self-contained.

Supersedes #1743.

---------

Co-authored-by: Richard Levasseur <[email protected]>
@aignas aignas deleted the feat/config-settings-os-cpu branch October 17, 2024 02:51
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.

1 participant