internal(config_settings): make config_setting creation reusable #1750
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aignas
force-pushed
the
refactor/config-settings
branch
from
d2db50e
to
1a1b191
Compare
aignas
commented
Feb 12, 2024
aignas
commented
Feb 12, 2024
aignas
commented
Feb 12, 2024
rickeylev
reviewed
Feb 15, 2024
aignas
commented
Feb 29, 2024
There was a problem hiding this comment.
Self review done, @rickeylev, I think this can be reviewed for a second time.
aignas
added a commit
to aignas/rules_python
that referenced
this pull request
Mar 10, 2024
This is a variant of bazelbuild#1625 and was inspired by bazelbuild#1788. In bazelbuild#1625, we attempt to parse the simple API HTML files in the same `pip.parse` extension and it brings the follownig challenges: * The `pip.parse` cannot be easily use in `isolated` mode and it may be difficult to implement the isolation if bazelbuild/bazel#20186 moves forward. * Splitting the `pypi_index` out of the `pip.parse` allows us to accept the location of the parsed simple API artifacts encoded as a bazel label. * Separation of the logic allows us to very easily implement usage of the downloader for cross-platform wheels. * The `whl` `METADATA` might not be exposed through older versions of Artifactory, so having the complexity hidden in this single extension allows us to not increase the complexity and scope of `pip.parse` too much. * The repository structure can be reused for `pypi_install` extension from bazelbuild#1728. TODO: - [ ] Add unit tests for functions in `pypi_index.bzl` bzlmod extension if the design looks good. - [ ] Changelog. Out of scope of this PR: - Further usage of the downloaded artifacts to implement something similar to bazelbuild#1625 or bazelbuild#1744. This needs bazelbuild#1750 and bazelbuild#1764. - Making the lock file the same on all platforms - We would need to fully parse the requirements file. - Support for different dependency versions in the `pip.parse` hub repos based on each platform - we would need to be able to interpret platform markers in some way, but `pypi_index` should be good already. - Implementing the parsing of METADATA to detect dependency cycles. - Support for `requirements` files that are not created via `pip-compile`. - Support for other lock formats, though that would be reasonably trivial to add. Open questions: - Support for VCS dependencies in requirements files - We should probably handle them as `overrides` in the `pypi_index` extension and treat them in `pip.parse` just as an `sdist`, but I am not sure it would work without any issues.
2 tasks
rickeylev
reviewed
Mar 15, 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
force-pushed
the
refactor/config-settings
branch
from
0565a23
to
67a3e8a
Compare
rickeylev
approved these changes
Mar 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
bzlmodif someone built Python fora platform that we don't provide a toolchain for and tried to use the
pip.parsemachinery with that by providing thepython_interpreter_target. That is a niche usecase, butrules_pythonis 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_settingsvalues and how they are constructed by adding an extrais_python_config_settingmacro, that could be used to declare theconfig settings from within the
pip.parsehub repo. This makes thework in #1744 to support whl-only hub repos more self-contained.
Supersedes #1743.