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

Avoid downloading protobuf dependancy when not needed #2543

Open
harrysarson opened this issue Jan 2, 2025 · 4 comments
Open

Avoid downloading protobuf dependancy when not needed #2543

harrysarson opened this issue Jan 2, 2025 · 4 comments
Labels
help wanted need: upstream support An issue that needs changes in upstream code

Comments

@harrysarson
Copy link

Hi we recently migrated a project from workspace to bzlmod and noticed our builds now pull in more things from the internet as a result.
In particular we saw that our builds started to download protobuf (from https://github.com/protocolbuffers/protobuf/releases/download/v29.0-rc2/protobuf-29.0-rc2.zip).
This was despite none of our targets referencing (either directly nor indirectly) py_proto_library (or any other protobuf target from rules_python or any other module).

It seems that in a workspace project dependencies are fetched lazily [1] whereas in a bzlmod project, dependencies are fetched eagerly.

Reading the source of this library, I saw a comment in the MODULE.bzl

# Those are loaded only when using py_proto_library
that suggests the protobuf dep will be loaded only if the py_proto_library is used. However, that doesn't match my tests where protobuf is download by all projects that depend on rules_python even if they do not use any rules from it [2].

I am wondering

  1. if there is a way to use rules_python without pulling protobuf if we do not need protobuf support?
  2. If the comment linked above needs updating/removing?

Related

I think #2498 is similar although that is about rules_kotlin and is more about toolchains fetched as module extensions rather than modules downloaded directly.

Notes

1: https://bazel.build/extending/repo#when_is_the_implementation_function_executed
2: You can test this by creating a custom registry (https://bazel.build/external/registry) and modifying the source.json file of all the protobuf modules to set the url field to something that does not exist. Building a bzlmod project that uses rules_python but does not have any py_proto_library targets will then fail showing that bazel does try to fetch protobuf.

@aignas aignas added help wanted need: upstream support An issue that needs changes in upstream code labels Jan 14, 2025
@aignas
Copy link
Collaborator

aignas commented Jan 14, 2025

I think when we were introducing bazel 8.0 support we had to bump rules_cc and protobuf deps and I think we may need support for rules_cc and other rulesets to not use .bzl files from rules_cc that would pull the protobuf deps.

#2293 is a related PR that should have fixed the behaviour you see, but it seems that it was not enough.

+1 on the idea, but help here would be appreciated.

aignas added a commit to aignas/rules_python that referenced this issue Jan 25, 2025
With this PR we attempt to remove our implementation of py_proto_library
and we just forward the calls to the upstream.

Hopefully this fixes bazelbuild#2543.
@aignas
Copy link
Collaborator

aignas commented Jan 25, 2025

I have created #2581, but I am not sure if it is going to fix the issue you are seeing, could you please test the PR?

@alexeagle
Copy link
Contributor

@aignas it's possible to write an assertion that "the following external repo was never fetched during this e2e test" - here's an example I wrote https://github.com/theoremlp/rules_multitool/blob/main/examples/workspace/minimal_download_test.sh which (ab)uses the Bazel Downloader Config to fast-fail if attempting to fetch something (protobuf in this case)

@aignas
Copy link
Collaborator

aignas commented Jan 28, 2025

I am not sure how much time I'll have to implement this suggestion, but I like the idea of an integration test. If you or someone interested in solving this ticket has time to contribute, please add me as a reviewer.

We try to have as few such integration tests as possible, but I don't really see how we could test this in a different way.

github-merge-queue bot pushed a commit that referenced this issue Feb 11, 2025
Protobuf team is taking ownership of `py_proto_library` and the
implementation was moved to protobuf repository.

Remove py_proto_library from rules_python, to prevent divergent
implementations.

Make a redirect with a deprecation warning, so that this doesn't break
any users.

Previously this was attempted in:
d0e25cf


Work towards #2173,
#2543
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted need: upstream support An issue that needs changes in upstream code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants