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

Add additional modules to module_ctx #22070

Open
matts1 opened this issue Apr 22, 2024 · 2 comments
Open

Add additional modules to module_ctx #22070

matts1 opened this issue Apr 22, 2024 · 2 comments
Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests good first issue help wanted Someone outside the Bazel team could own this P2 We'll consider working on this in future. (Assignee optional) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: feature request

Comments

@matts1
Copy link
Contributor

matts1 commented Apr 22, 2024

Description of the feature request:

I'd like additional properties on module_ctx.

  • module_ctx.root_module would be an Optional[bazel_module] that would be equivalent to the output of the _find_root function below
  • module_ctx.current_module would be a bazel_module object corresponding to the module that defined the current module extension (it could possibly be a Optional[bazel_module].

A classic piece of code being written is:

def _find_root(module_ctx):
  for mod in module_ctx.module:
      if mod.is_root:
          return mod

def _find_current_module(module_ctx):
  for mod in module_ctx.module:
      if mod.name == "rules_rust":
          return mod

This would significantly simplify the recommended workflow we prescribe in #22024 (eg. bazelbuild/rules_rust#2624)

Which category does this issue belong to?

Core, External Dependency

What underlying problem are you trying to solve with this feature?

Our discussion about the right way to implement toolchains in bazel came to the following conclusion:

The policy would be, for a given toolchain defined by rules_foo which has the tag foo.toolchain:

  • If the root module defines a toolchain tag, the toolchain configuration will be:
    • Configured based on the root MODULE's foo.toolchain tag
      • Any unspecified fields may be inherited from rules_foo's foo.toolchain
      • Any specified fields must not be affected by rules_foo's foo.toolchain
      • The toolchain configuration must not be affected by any module other than the root module and rules_foo
    • If MVS is supported, rules_foo may choose to compare the root MODULE's foo.toolchain with MVS(all modules' foo.toolchain) in order to output warnings
  • If the root module doesn't define a toolchain, the toolchain is defined by:
    • If MVS is supported, MVS(all modules' foo.toolchain invocations)
    • Otherwise, rules_foo's foo.toolchain
  • Non-leaf modules should specify the minimum possible versions and minimal set of features in their foo.toolchain

This will require special-casing of the root module and the current module extension, both of which seems like something pretty reasonable to me.

Which operating system are you running Bazel on?

linux

What is the output of bazel info release?

7.1.1

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@github-actions github-actions bot added team-Core Skyframe, bazel query, BEP, options parsing, bazelrc team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. labels Apr 22, 2024
@Wyverald Wyverald added P2 We'll consider working on this in future. (Assignee optional) good first issue help wanted Someone outside the Bazel team could own this area-Bzlmod Bzlmod-specific PRs, issues, and feature requests and removed untriaged team-Core Skyframe, bazel query, BEP, options parsing, bazelrc labels Apr 22, 2024
@Wyverald
Copy link
Member

This sounds good to me (except maybe if we can find a better name for current_module).

The implementation is quite simple (file in question: https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java), so contributions are welcome.

@fmeum
Copy link
Collaborator

fmeum commented Apr 22, 2024

In the module extensions I have worked on so far, the usual pattern has been:

def _impl(module_ctx):
  collected_data = ...

  for mod in module_ctx.module:
      if mod.is_root:
          do_stuff(collected_data)

      do_other_stuff(collected_data)

I can see the new helpers being useful in smaller module extensions, but I think that many (most?) extensions will want to do something (even if just validation) with all modules, not just root and "current" module. The new symbols would get us into a situation where do_something(module_ctx.root_module), if mod.is_root: do_something(mod) and if mod == module_ctx.root_module: do_something(mod) are all different ways to do the same thing.

I am a bit worried that introducing new API that doesn't provide any truly new information could end up making module_ctx more confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests good first issue help wanted Someone outside the Bazel team could own this P2 We'll consider working on this in future. (Assignee optional) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: feature request
Projects
None yet
Development

No branches or pull requests

6 participants