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

Support gazelle plugin when using bzlmod #965

Closed
aignas opened this issue Jan 3, 2023 · 7 comments · Fixed by #1077
Closed

Support gazelle plugin when using bzlmod #965

aignas opened this issue Jan 3, 2023 · 7 comments · Fixed by #1077

Comments

@aignas
Copy link
Collaborator

aignas commented Jan 3, 2023

🚀 feature request

Creating this issue to encompass all of the PRs related to this feature, as the single research PR (#961) got a little bit too big for easy review.

Relevant Rules

gazelle Python plugin.

Description

Currently the gazelle plugin does not work if using bzlmod with bazel >= 6.0.0. This is due to:

  • Gazelle and go toolchain is not present in MODULE.bazel of rules_python, which means that the plugin reference fails.
  • The all_whl_requirements have labels, that do not work in bzlmod due to repository remapping. Hence, the modules_mapping rule fails unless we do something about it.
  • Gazelle will generate dependency labels in the targets that do not work in bzlmod due to repository remapping.

Describe the solution you'd like

As researched in #961, we can fix these three issues separately:

  1. Adding gazelle and go as bzlmod deps.
  2. Changing the contents of the all_whl_requirements and all_requirements based on wether bzlmod is used.
  3. Have extra declaratives for changing the generated labels to work with bzlmod.
  4. Have extra aliases in the @pip repo that work with both, bzlmod and classic gazelle. This might be similar to feat: add alias generation for pip parse #814, however this is not a requirement to make gazelle with bzlmod work.

Describe alternatives you've considered

Rejected alternatives:

  • Having a flag in gazelle_python.yaml manifest, which would say that we are using bzlmod. The manifest should be the same irrespective of whether we are using bzlmod or not due to the same reason why we may want to have aliases in @pip repo -- because we want to enable a smooth transition to bzlmod as described in the migration guide
aignas added a commit to aignas/rules_python that referenced this issue Jan 3, 2023
This changes the label strings that the users would get when loading the
`all_requirements` and `all_whl_requirements` from the generated
`requirements.bzl`. Previous labels were not really useable under
bzlmod, because they would not be visible to the consumer.

Work towards bazelbuild#965
aignas added a commit to aignas/rules_python that referenced this issue Jan 3, 2023
Allow setting conventions for each part of the external repo labels.
This is also useful when creating multi-platform alias targets when
using an approach described in 2022 bazelcon [1].

[1]: https://www.youtube.com/watch?v=Bjaiu8tZZhs

Work towards bazelbuild#965.
aignas added a commit to aignas/rules_python that referenced this issue Jan 3, 2023
@aignas aignas mentioned this issue Jan 3, 2023
12 tasks
aignas added a commit to aignas/rules_python that referenced this issue Jan 3, 2023
aignas added a commit to aignas/rules_python that referenced this issue Jan 3, 2023
@aignas
Copy link
Collaborator Author

aignas commented Jan 3, 2023

Some ideas that came from the comment in #970.

Regarding the metadata for gazelle to consume from the requirements.bzl file, I see that there are 2 pieces for gazelle to work:

  • modules_mapping creates a mapping between import names and Python package names. It downloads the whl files to get this information, so it depends on the said whl targets.
  • gazelle_python.yaml manifest generation depends on the mapping and a few extra variables, which help it map the Python package name back to the bazel label for the pkg target.

Thinking from outside-in, I'd like to use something like below in my BUILD.bazel files:

# BUILD.bazel contents
load("@pip//:gazelle.bzl", "gazelle_python_manifest")
load("@gazelle//:def.bzl", "gazelle", "gazelle_binary")
load("@rules_python//gazelle:defs.bzl", "GAZELLE_PYTHON_RUNTIME_DEPS")

# This macro could read the metadata within the '@pip//:gazelle.bzl' which would have the
# following info:
# - The whl library list needed for gazelle.
# - The name of the pip repository.
# - The pip repo naming convention, thus eliminating the need for some of the
#   PRs in the series.
# - The '//:requirements_lock.txt' could be also infered as we pass it to `pip_parse` anyway.
gazelle_python_manifest(
    name = "gazelle_python_manifest",
    extend_exclude_patterns = [
        "(\\tests)+",  # There are many packages on PyPI with tests in their wheels
    ],
)

gazelle_binary(
    name = "gazelle_bin",
    extend_languages = [
        "@rules_python//gazelle",
    ],
)

gazelle(
    name = "gazelle",
    data = GAZELLE_PYTHON_RUNTIME_DEPS,
    binary = ":gazelle_bin",
)

I'll play around with the idea of having gazelle macro come from the output of pip_parse.

@alexeagle
Copy link
Collaborator

For your #1, rules_python must not introduce a Go dependency for all users. The situation is the same as for bazel-skylib, which has a Gazelle extension for creating bzl_library targets. The solution there is a submodule introduced in bazelbuild/bazel-skylib@60abca8#diff-7490f090fcbdb5d3b486333531265e0f0ad157dd5cadc964b3c98698021f3ced so I suspect rules_python should have something similar.

@aignas
Copy link
Collaborator Author

aignas commented Jan 3, 2023

Thanks @alexeagle, will change the approach in #968. I was under an impression, that the dependencies will be lazy-fetched anyway, so there is no harm.

aignas added a commit to aignas/rules_python that referenced this issue Jan 4, 2023
aignas added a commit to aignas/rules_python that referenced this issue Jan 4, 2023
aignas added a commit to aignas/rules_python that referenced this issue Jan 4, 2023
This changes the label strings that the users would get when loading the
`all_requirements` and `all_whl_requirements` from the generated
`requirements.bzl`. Previous labels were not really useable under
bzlmod, because they would not be visible to the consumer.

Work towards bazelbuild#965
aignas added a commit to aignas/rules_python that referenced this issue Jan 4, 2023
aignas added a commit to aignas/rules_python that referenced this issue Jan 4, 2023
aignas added a commit to aignas/rules_python that referenced this issue Jan 4, 2023
This changes the label strings that the users would get when loading the
`all_requirements` and `all_whl_requirements` from the generated
`requirements.bzl`. Previous labels were not really useable under
bzlmod, because they would not be visible to the consumer.

Work towards bazelbuild#965
aignas added a commit to aignas/rules_python that referenced this issue Jan 4, 2023
aignas added a commit to aignas/rules_python that referenced this issue Jan 4, 2023
@aignas
Copy link
Collaborator Author

aignas commented Jan 4, 2023

@alexeagle, #972 moves gazelle plugin into a separate workspace, other PRs will add bzlmod to it. Let me know if this is something similar to what you had in mind.

@aignas
Copy link
Collaborator Author

aignas commented Jan 11, 2023

It seems that the approach described in one of my first comments should be changed given that there is a tool for easily managing the use_repo repositories. So the proposed plan would be to:

  1. breaking: Move gazelle plugin into a separate WORKSPACE (feat(gazelle)!: Move the plugin to a separate workspace #972).
  2. Add a MODULE.bazel file with correct dependencies (feat(gazelle): support bzlmod #968).
  3. Onboard rules_python onto auto_use_repo tool. Add auto_use_repo support for go_deps bazel-gazelle#1404 could be a good starting point.
  4. Deprecate the current aliased targets which are in the main pip_parse repo when using bzlmod. People should use auto_use_repo to manage the use_repo(pip, ...) statement to consume pip repositories, thus making the targets identical between bzlmod and non-bzlmod cases. This can be done in a future release as it is not hindering usage of gazelle from bzlmod.
  5. Remove the aliased targets which are created in the main pip repo when using bzlmod.

The changes in #971, #970, #967, #966 would be not needed if auto_use_repo is adopted. Let me know what you think about such new direction.

@alexeagle
Copy link
Collaborator

My understanding is that auto_use_repo is still experimental, and only needed in rules_go or similar cases where users typically depend on a package nested within the dependency. For Python, users depend on entire packages, so their MODULE.bazel files do not need use_repo lines for every dependency (and they make maintenance of that file harder).

Instead gazelle should load through the @pip repository (or whatever the user named it).

aignas added a commit to aignas/rules_python that referenced this issue Jan 12, 2023
This is in order to make bazelbuild#972 easier to review. This PR is only moving
files and addressing a few small review comments made in the initial
review of bazelbuild#972.

Work towards bazelbuild#965.
aignas added a commit to aignas/rules_python that referenced this issue Jan 12, 2023
This is in order to make bazelbuild#972 easier to review. This PR is only moving
files and addressing a few small review comments made in the initial
review of bazelbuild#972.

Work towards bazelbuild#965.
aignas added a commit to aignas/rules_python that referenced this issue Jan 12, 2023
This is in order to make bazelbuild#972 easier to review. This PR is only moving
files and addressing a few small review comments made in the initial
review of bazelbuild#972.

Work towards bazelbuild#965.
aignas added a commit to aignas/rules_python that referenced this issue Jan 24, 2023
This is in order to make bazelbuild#972 easier to review. This PR is only moving
files and addressing a few small review comments made in the initial
review of bazelbuild#972.

Work towards bazelbuild#965.
aignas added a commit to aignas/rules_python that referenced this issue Jan 27, 2023
This changes the label strings that the users would get when loading the
`all_requirements` and `all_whl_requirements` from the generated
`requirements.bzl`. Previous labels were not really useable under
bzlmod, because they would not be visible to the consumer.

Work towards bazelbuild#965
@aignas
Copy link
Collaborator Author

aignas commented Feb 13, 2023

So given that #1043 has gone through a review process, I went ahead and created a POC gazelle support for bzlmod. It is currently in https://github.com/aignas/rules_python/tree/gazelle-bzlmod.

Having a backwards compatible API is important and having 2 flags that can be passed to gazelle_manifest_update and pip_parse which would work identically under bzlmod and the old way was the key to unlock everything. I would like to wait until #1065 is merged, so that I can have more confidence that we don't regress older bazel versions.

The whole architecture is:

  • Allow generating package aliases in the pip_parse repository with names @pip//<pkg>. This is gated under incompatible_generate_aliases, which I used because it necessitates the user to regenerate the build files using bazel run //:gazelle and potentially fix the resolve py directives if they exist.
  • Add an optional parameter in the gazelle_manifest_update, for starting to use @pip//<pkg> labels. If the user doesn't set it, the behaviour is unchanged.
  • Add MODULE.bazel for the gazelle plugin and the build_file_generation example and it worked after tinkering a little bit.

The change is still a little big for my tastes, but not sure yet what would be the best way to split it for easy review.

8       5       docs/pip_repository.md
4       1       examples/build_file_generation/BUILD.bazel
43      0       examples/build_file_generation/MODULE.bazel
2       0       examples/build_file_generation/WORKSPACE
2       15      examples/build_file_generation/gazelle_python.yaml
20      0       gazelle/MODULE.bazel
13      44      gazelle/deps.bzl
0       2       gazelle/go.mod
10      1       gazelle/manifest/defs.bzl
16      7       gazelle/manifest/generate/generate.go
3       0       gazelle/manifest/manifest.go
0       1       gazelle/python/BUILD.bazel
8       2       gazelle/pythonconfig/pythonconfig.go
1       0       python/extensions.bzl
67      5       python/pip_install/pip_repository.bzl

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 a pull request may close this issue.

2 participants