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

Dependency on rules_kotlin (due to protobuf and stardoc) #2498

Closed
jacky8hyf opened this issue Dec 11, 2024 · 6 comments
Closed

Dependency on rules_kotlin (due to protobuf and stardoc) #2498

jacky8hyf opened this issue Dec 11, 2024 · 6 comments

Comments

@jacky8hyf
Copy link

🐞 bug report

Affected Rule

Dependency on stardoc & protobuf cause this issue.

Is this a regression?

Yes, the previous version in which this bug was not present was: 0.38.0

Description

Due to stardoc 0.7.x and protobuf 29.0 dependency, rules_kotlin is fetched. Even though there are no targets directly depending on rules_kotlin, I think it is fetched probably because rules_kotlin contains a toolchain declaration.

🔬 Minimal Reproduction

Set up the workspace like the following:

==> ./MODULE.bazel <==
module(name = "hah")

bazel_dep(
    name = "rules_python",
    version = "0.39.0",
)

# local_path_override(
#     module_name = "protobuf",
#     path = "protobuf",
# )

# local_path_override(
#     module_name = "stardoc",
#     path = "stardoc",
# )

==> ./BUILD.bazel <==
load("@rules_python//python:py_library.bzl", "py_library")

py_library(
    name = "foo",
    srcs = [],
)

==> ./stardoc/MODULE.bazel <==
module(name = "stardoc")

==> ./protobuf/MODULE.bazel <==
module(name = "protobuf")

==> ./protobuf/bazel/cc_proto_library.bzl <==
cc_proto_library = None

==> ./protobuf/bazel/BUILD.bazel <==

==> ./protobuf/bazel/java_proto_library.bzl <==
java_proto_library = None

==> ./protobuf/bazel/java_lite_proto_library.bzl <==
java_lite_proto_library = None

A tarball is also attached for quick repo:
workspace.tar.gz

With Bazel 7.4.0 (see explanation below), run the following:

bazel clean --expunge && bazel build //:foo
ls $(bazel info output_base)/external/*rules_kotlin*

We can see that rules_kotlin is fetched.

Now open the root MODULE.bazel file and make the following edits, then re-run the aforementioned commands.

  • If rules_python were 0.38.0, rules_kotlin would not be fetched. This is because it relies on older stardoc and protobuf that does not need rules_kotlin.
  • If rules_python is 0.39.0, but local_path_override() for stardoc and protobuf were uncommented, then rules_kotlin would not be fetched. This is because rules_kotlin is no longer in the dependency tree.
  • If rules_python were modified locally (with local_path_override()) so that it cuts the protobuf dependency and sets the stardoc dependency as dev_dependency = True, then rules_kotlin is not fetched.

🌍 Your Environment

Operating System:

  
Linux
  

Output of bazel version:

  
Build label: 7.4.0
Build target: @@//src/main/java/com/google/devtools/build/lib/bazel:BazelServer
Build time: Tue Oct 22 17:24:25 2024 (1729617865)
Build timestamp: 1729617865
Build timestamp as int: 1729617865
  

Note: I intentionally pick 7.4.0 not the latest 8.0.0 because its bazel_tools has lower-version dependencies that won't affect our experiment with protobuf, rules_jvm_external and stardoc. With 8.0.0, Bazel mod version resolution always sets higher version for protobuf and rules_jvm_external, so we can't independently verify that the issue does not happen with rules_python@0.38.0.

Rules_python version:

  
0.39.0
0.40.0
1.0.0
  

Anything else relevant?

The core issues here is that rules_python depends on Protobuf and Stardoc, two very big repositories with a lot of dependencies, with PITA reasons.

For Protobuf, ideally py_proto_library should come from rules_proto not rules_python. AFAIK this is #2173.

For Stardoc, this appears to come from 5978390. Usually, Stardoc should be a dev_dependency so that the root module can choose whether it wants Stardoc. But rules_python provides sphinx_stardoc which requires a non-dev dependency on Stardoc. I can file a separate bug to track this if it makes more sense.

Both have heavy dependencies on Java and Kotlin, not to mention the Maven dependencies that Stardoc has. As the code grows, it is highly possible that Stardoc and Protobuf have even more dependencies in the future, where some may contain toolchains, causing users of rules_python to vendor them to have offline builds. I found it very funny that rules_python has Java dependencies.

Context: Android kernel builds are executed by Google's build servers and partner's build servers that cannot reach the Internet. Therefore, all dependencies must be imported into AOSP with careful reviews. Because the Android kernel build does not contain any Java, Protobuf (yet) or Kotlin code, it is hard for us to justify that rules_kotlin should be imported.

@jacky8hyf
Copy link
Author

To help isolating the issue, here's the output of bazel mod graph --include_builtin --verbose when rules_python is 0.38.0 and 0.39.0, respectively:

graph.0.38.0.txt
graph.0.39.0.txt

One thing to highlight is that rules_python (0.38.0 -> 0.39.0) means stardoc (0.6.2 -> 0.7.1) and protobuf (24.4 -> 29.0-rc2), which in turn means rules_jvm_external (5.2 -> 6.3), which adds the rules_kotlin@1.9.6 dependency.

@aignas
Copy link
Collaborator

aignas commented Dec 11, 2024

Could you explain why this can be a problem, please?

@jacky8hyf
Copy link
Author

Could you explain why this can be a problem, please?

This is in the end of the first comment:

Context: Android kernel builds are executed by Google's build servers and partner's build servers that cannot reach the Internet. Therefore, all dependencies must be imported into AOSP with careful reviews. Because the Android kernel build does not contain any Java, Protobuf (yet) or Kotlin code, it is hard for us to justify that rules_kotlin should be imported.

In other words, the core issues are the following:

  • rules_python is a fundamental dependency by all projects that uses Python. If rules_python has a lot of dependencies, the client projects using rules_python will need them. This is especially problematic for client projects that needs to be built in an air-gap environment. Therefore, we should minimize the dependency list of rules_python, just like rules_cc etc.
  • Though Bazel usually skips fetching modules that the requested target does not depend on, that is not the case for modules registering a toolchain (I am still unsure if that's the root cause for fetching rules_kotlin in this case). Fetching rules_kotlin surprises client projects because we need to bring rules_kotlin into the air-gap environment.

@rickeylev
Copy link
Collaborator

Though Bazel usually skips fetching modules that the requested target does not depend on, that is not the case for modules registering a toolchain

This is a good observation. That said, keep in mind we have limited control over this because we can only control our direct dependencies. Similarly, a module can't control over what toolchains are registered by other modules. Stated another way: I suspect this will be an ongoing issue for you because bzlmod makes it easier to register toolchains and take on dependencies.

To be clear, IMHO, Bazel is lacking something to better handle this situation -- this isn't the first time I've seen users have to deal with toolchains registered somewhere that they didn't want. Language rule authors are in a catch-22 because, if they don't register something by default, then things don't Just Work. If they do register something by default, then it can easily incur an expensive dependency/operate when users (like yourself) are going through extra steps to explicitly avoid that cost.

Another thing to try is move rules_python earlier in your module dependency graph. This will make its toolchain registrations come before e.g. rules_kotlin, which might side-step the issue.

Otherwise, patching MODULE files is probably your best alternative.

re: non-dev stardoc dependency: As mentioned in internal chat, I, too, would like to remove this as a non-dev dependency. Unfortunately, the code in //sphindocs won't work with it being a dev dependency. The only solution I can think of is to split that out into a separate bzlmod module. That is probably feasible if we split it out similar to how //gazelle is handled. I'd be +1 on that, but not sure when we'll have time to do that. I'm happy to review and walk through how to do that, though.

re: protobuf dependency: As mentioned in internal chat, this dependency can't be removed for the time being. The long term plan is to move py_proto_library out of rules_python, but, last I heard, some work in protobuf is still needed (not sure what). So, unfortunately, not much can be done about this part.

rules_python is a fundamental dependency by all projects that uses Python. If rules_python has a lot of dependencies, the client projects using rules_python will need them. This is especially problematic for client projects that needs to be built in an air-gap environment. Therefore, we should minimize the dependency list of rules_python, just like rules_cc etc.

I understand your point, but, well, this came across a bit rude. I'd appreciate a less proscribing tone :).

@jacky8hyf
Copy link
Author

Thanks for your explanation. I apologize for the offensive tone. I did not mean to be condescending or authoratative here. Sorry.

With trial and error, the following is the best workaround for us. It is written below in case anyone else come across a similar issue.

  • Adding local_path_override or --override_module for the unexpected dependency (rules_kotlin/protobuf in this case)
  • Create a stub for it so the directory only contains a single MODULE.bazel with a single module(name = "rules_kotlin") line

I am closing this bug now because we (Android kernel team) don't need any changes from rules_python at this moment, but feel free to reopen/refer to this bug if necessary!

@jacky8hyf jacky8hyf closed this as not planned Won't fix, can't repro, duplicate, stale Dec 17, 2024
@aignas
Copy link
Collaborator

aignas commented Dec 17, 2024

Thank you for the update here. I will create a separate ticket for splitting out the sphinxdoc module and tag it with help wanted.

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

No branches or pull requests

3 participants