Gazelle: Sanitize distribution names in modules mapping / manifest, not at runtime #1939
Comments
|
A few thoughts here before I forget :) I think that returning the mapping of import to python package from Being able to have a Regarding Being devil's advocate, you could also argue that changing the |
There was an attempt here jvolkman/rules_pycross#89. It gives some clues why the author decided not to accept it. Basically, it is inverse to your point that pycross ideas shouldn't be bend for the sake of satisfying another plugin.
I somewhat agree, but someone should make a first step unless the goal is to keep Gazelle plugin a closed garden I guess :) I'd say it should be the side where it gives a decoupling effect (Gazelle plugin) rather than an ugly bridge/adapter (rules_pycross). Perhaps I see it is incorrectly, but How about having a with default value of |
|
On Mon Jun 10, 2024 at 8:51 PM JST, Illia Ovchynnikov wrote:
> Creating a wrapper around rules_pycross hub repo, that would externally look like rules_python one is not too difficult (but tedious) if there is a known naming scheme. So doing that could be another workaround to consider.
There was an attempt here jvolkman/rules_pycross#89. It gives some clues why the author decided not to accept it. Basically, it is inverse to your point that pycross ideas shouldn't be bend for the sake of satisfying another plugin.
This makes sense, though it's the ruleset choice of:
1. Support gazelle rules_python plugin.
2. Create a new gazelle rules_pycross plugin.
3. Not support gazelle at all.
And given that quite a lot of these projects are based on volunteer
time, I think all three are valid choices. :)
> You could also argue that changing the gazelle plugin to ensure that rules_pycross integrates well with the rules_python gazelle plugin is a code smell.
I somewhat agree, but someone should make a first step unless the goal is to keep Gazelle plugin a closed garden I guess :) I'd say it should be the side where it gives a decoupling effect (Gazelle plugin) rather than an ugly bridge/adapter (rules_pycross).
Perhaps I see it is incorrectly, but `gazelle_python.yaml` to me is/should be just a lookup dict for Gazelle plugin. While I agree it is better to keep `modules_mapping ` as is, I'd see any target transformation/normalization happening on `gazelle_python.yaml` generation which the plugin should afterwards take as a 1:1 mapping and refrain from adding any configuration to alter runtime behaviour there. Gazelle directives sounds like a more common/appropriate place to alter Gazelle (incl. plugins) behaviour to me.
The rules_python_gazelle_plugin is a gazelle plugin, so directives
could still work here? I am not sure here why you put the distinction
here the way you do. Could you give any other examples of plugins, that
you have in mind, so that I better understand what you suggest?
Right now I see `gazelle_python.yaml` and directives as two alternatives
and I am not sure if one is much better than the other right now.
How about having a `target_convention` attr to `gazelle_python_manifest`? Something like:
```
gazelle_python_manifest(
name = "requirements_mapping",
modules_mapping = ":requirements_metadata",
pip_repository_name = "pip",
target_convention = "{{ printf "%s//:%s" $pip_repository_name (regexReplaceAll "[-._]+" (lower $target) "-") }}",
requirements = ":requirements_txt",
)
```
with default value of `{{ printf "%s//%s" $pip_repository_name (regexReplaceAll "[_\\.]" (lower $target) $target "_") }}`. `pip_repository_name` attr might also be deprecated here to fully use `target_convention` while keeping backwards compatibility. `target_convention` can use [text/template](https://pkg.go.dev/text/template) with extra functions like `regexReplaceAll` from sprig, or it could be some custom simplified version of it.
Are you suggesting to change how the `gazelle_python.yaml` is
generated, where we would have:
```yaml
manifest:
modules_mapping:
<import path>: <some part of the label that the import belongs to>
pip_repository:
name: pip
integrity: deadbeef
```
I am not sure I am sold on this idea to have the second option, but I am
open to having the first option where we extend the schema.
The simplest way to solve this whilst still keeping it generic could be
to extend the `gazelle_python.yaml` schema or have extra directives to:
```yaml
manifest:
modules_mapping:
<import path>: <some part of the label that the import belongs to>
pip_repository:
name: pip
label_convention: $repo//$distribution # default
normalization: <none|kebab-case|snake_case> # we would default to snake_case
integrity: deadbeef
```
When I was initially porting the `gazelle` plugin to work with the `hub`
pattern that is used in `bzlmod`, I thought that having some sort of
convention parameter would be a good way to customize things.
I think in my head we could be opinionated and support only a limited
set of normalization schemes that are simple and make it hard to
misconfigure. Having an enum fits the bill as it is easy to validate and
provide easy to understand error messages. What is more there are only
three sensible options here:
1. Do nothing with the python package names at all.
2. Do what `rules_python` is doing (because that is a standard that is
ingrained in many places, not saying it is good, but it is one).
2. Normalized based on the Python packaging guidelines.
This is also to ensure that support for case sensitive and case
insensitive OSes is equally good.
What do you think? Doing simple substitution in the `label_convention`
and having three values of normalization sounds like a non-invasive
change.
|
|
What I was trying to say w.r.t. I like the idea of
Alternatively, it could be However, I would put it into a directive, rather than No changes to |
|
Sounds great! Keeping Bike shedding a little here:
|
Adds new directives to alter default Gazelle label format to third-party dependencies useful for re-using Gazelle plugin with other rules, including `rules_pycross`. Fixes #1939
🚀 feature request
Relevant Rules
modules_mappinggazelle_python_manifestgazelleDescription
Gazelle plugin uses
gazelle_python.yaml(gazelle:python_manifest_file_name) to derivedepsfor py targets. One might expect that a dependency label will be${manifest.pip_repository}//${manifest.modules_mapping.some.import}, however, Gazelle does extra sanitisation right before using the mapping value here.Although it is not a big deal when used with
rules_python, it makes harder to reuse Gazelle plugin with other python rules, more specifically rules_pycross, which has a different target naming convention, which is aligned with Python normalised names. In this ticket, there is an approach (or hack really) described for patchinggazelle_python.yamlbut further sanitisation that happens inside the plugin itself makes it impossible to use.I do understand that compatibility with other rules most likely isn't of highest priority, here are some other reasons:
modules_mappingand/orgazelle_python_manifestand/or plugin itself.Describe the solution you'd like
SanitizeDistributioninpythonconfig.Config#FindThirdPartyDependency, take the value as is.SanitizeDistributioninmodules_mapping, orgazelle_python_manifest. I'd saymodules_mappingis best but please advice.The text was updated successfully, but these errors were encountered: