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

Improve googleapis use_languages extension tag #3437

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mering
Copy link
Contributor

@mering mering commented Dec 16, 2024

This obeys requested bindings from any module.

@bazel-io
Copy link
Member

Hello @bazelbuild/bcr-maintainers, modules without existing maintainers (googleapis) have been updated in this PR. Please review the changes.

@mering
Copy link
Contributor Author

mering commented Dec 16, 2024

@bazel-io skip_check unstable_url

@bazel-io bazel-io added the skip-url-stability-check Skip the URL stability check for the PR label Dec 16, 2024
@mering mering mentioned this pull request Dec 16, 2024
@mering mering force-pushed the googleapis-use-languages branch 3 times, most recently from 947450d to 6c7eb25 Compare December 16, 2024 18:53
@mering mering changed the title Improve use_languages extension tag Improve googleapis use_languages extension tag Dec 16, 2024
@mering
Copy link
Contributor Author

mering commented Dec 17, 2024

@meteorcloudy @fmeum @keith could you PTAL? Thanks!

fmeum
fmeum previously approved these changes Dec 17, 2024
This obeys requested bindings from any module.
@mering mering force-pushed the googleapis-use-languages branch from 6c7eb25 to 5bda46c Compare December 17, 2024 10:34
@bazel-io bazel-io dismissed fmeum’s stale review December 17, 2024 10:34

Require module maintainers' approval for newly pushed changes.

@mering
Copy link
Contributor Author

mering commented Dec 17, 2024

@fmeum Tests also succeeded for Bazel 8.x.

@fmeum
Copy link
Contributor

fmeum commented Dec 17, 2024

Having thought more about this, I do have some concerns about this. By allowing BCR modules to start using googleapis as is, we are making it more difficult for us to migrate the module to a reasonable structure: It has to be split into a module per language.

@meteorcloudy @Wyverald Is official Bzlmod support for googleapis on the maintainers' roadmap?

@mering
Copy link
Contributor Author

mering commented Dec 17, 2024

Having thought more about this, I do have some concerns about this. By allowing BCR modules to start using googleapis as is, we are making it more difficult for us to migrate the module to a reasonable structure: It has to be split into a module per language.

@meteorcloudy @Wyverald Is official Bzlmod support for googleapis on the maintainers' roadmap?

There are already various BCR modules using googleapis. This basically just allows us to test these targets in the BCR CI as well. If you prefer modules using googleapis without tests on BCR in the meantime please let me know.

@fmeum
Copy link
Contributor

fmeum commented Dec 17, 2024

As far as I understand the module extension logic in the current version, only the root module can enable languages. For languages such as C#, even the root module likely can't do that since googleapis doesn't declare the required bazel_deps.

Presumably this means that any user of a module that transitively uses googleapis needs to include a switched_rules_by_language usage in their root module file. Can you do the same to run your tests in BCR CI? If not, how do your tests differ from what a regular use of this module would look like? I'm pretty sure we can get you decent test coverage somehow without "prod changes".

The problem with module extensions that do something for non-root modules is that they very quickly form public API that is very difficult to change - there is no way to have bazel_features like logic in module files and even max_compatibility_level doesn't allow a module to be compatible with two different versions of a module extension. Since googleapis doesn't even come with upstream support for Bzlmod yet, we would severely limit this future support if we patch in this kind of API now. Based on my experience with googleapis so far, it has been a constant source of big trouble and without a better API than we have right now (including with this PR), we could end up cementing that state even with Bzlmod.

@mering
Copy link
Contributor Author

mering commented Dec 17, 2024

Presumably this means that any user of a module that transitively uses googleapis needs to include a switched_rules_by_language usage in their root module file. Can you do the same to run your tests in BCR CI? If not, how do your tests differ from what a regular use of this module would look like? I'm pretty sure we can get you decent test coverage somehow without "prod changes".

How can I tell the BCR CI to add a call to switched_rules_by_language call to the testing module? The CI doesn't run th tests in the module itself but creates a separated testing module only depending on the uploaded module for tests.

The problem with module extensions that do something for non-root modules is that they very quickly form public API that is very difficult to change - there is no way to have bazel_features like logic in module files and even max_compatibility_level doesn't allow a module to be compatible with two different versions of a module extension. Since googleapis doesn't even come with upstream support for Bzlmod yet, we would severely limit this future support if we patch in this kind of API now. Based on my experience with googleapis so far, it has been a constant source of big trouble and without a better API than we have right now (including with this PR), we could end up cementing that state even with Bzlmod.

Where is the difference to requiring this in the root module? In both cases, one needs to configure the extension. Let's assume in the future the call to switched_rules_by_language is no longer needed for example. So one could just make it a noop. Let's assume there is any other way to enable languages, then one can just call the other function to enable the languages from the previous extension. So the only public API is the use_languages extension tag which already exists before this PR.

@fmeum
Copy link
Contributor

fmeum commented Dec 17, 2024

It's quite possible that the new "API" will be that there are separate googleapi-<lang> modules that one needs to explicitly depend on. That's not a change that can be performed in a backwards compatible way. If only root modules need to change, that's a very manageable change, if it transitively breaks BCR modules, that will result in far more pain.

I'm not fully against making this change, but I would like to give the maintainers of the upstream repo a say in this.

@mering
Copy link
Contributor Author

mering commented Dec 17, 2024

It's quite possible that the new "API" will be that there are separate googleapi-<lang> modules that one needs to explicitly depend on. That's not a change that can be performed in a backwards compatible way. If only root modules need to change, that's a very manageable change, if it transitively breaks BCR modules, that will result in far more pain.

I'm not fully against making this change, but I would like to give the maintainers of the upstream repo a say in this.

This would also break transitive modules with the current version as they require the targets to exist. When they are moved to separate modules, those transitive dependency modules would equally break.
Currently, only the root module triggers the generation of the language specific bindings while other modules can already use those generated bindings.

@fmeum
Copy link
Contributor

fmeum commented Dec 17, 2024

This would also break transitive modules with the current version as they require the targets to exist. When they are moved to separate modules, those transitive dependency modules would equally break.

That's not necessarily the case: We could arrange this in a way where users depending on googleapis-java communicates to googleapis via a module extension that the Java variant is available. googleapis could then forward the Java targets that currently exist to the Java "overlay" through aliases. In this way, we could maintain backwards compatibility with transitive usages.

You are right that the module extension could become a noop in the future. I don't enough about googleapis to determine whether the contents of com_google_googleapis_imports are relevant for other modules. If they are and this change would make it possible to control the repo's contents, that would create new concerns as a true noop implementation would no longer do it. Do you know what it's used for?

@mering
Copy link
Contributor Author

mering commented Dec 17, 2024

I don't enough about googleapis to determine whether the contents of com_google_googleapis_imports are relevant for other modules. If they are and this change would make it possible to control the repo's contents, that would create new concerns as a true noop implementation would no longer do it. Do you know what it's used for?

I don't know what exactly it is used for but I know that a depending module doesn't need the use_repo() to com_google_googleapis_imports and can still use the language specific proto bindings. So this seems to be used internally only.

@mering
Copy link
Contributor Author

mering commented Dec 18, 2024

I just want to clarify that this change:

  • does not change the way how non-root modules can use googleapis
  • mainly relieves the burden for the root module to specify desired googleapis languages for transitive dependencies
  • does not break any existing usages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-url-stability-check Skip the URL stability check for the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants