Observations about bzlmod

[Lukács T. Berki]

Hey Xudong,

I finally managed to review the progress of bzlmod before making it generally available and I have to say, it's excellent work; I am not wise enough to tell whether it will work out as a full replacement for WORKSPACE files, but this is about the best attempt I can imagine that's humanly possible so I'll do my best to move the needle along. I especially like the effort that went into making it possible to gradually migrate from WORKSPACE files.

I have a few observations, none showstoppers and quite possibly some of them are only due to my ignorance and probably quite some of them can't be changed without much sweat anyways, but it's better to discuss them now before bzlmod becomes officially supported.

Underscore as name component separator: it looks like you settled upon using underscore as the name component separator so that the namespace of module names can be hierarchical. This is somewhat unfortunate because underscore is used in many places, but I guess it's too late to change now, so meh. There isn't an obviously better choice anyway.

--ignore_dev_dependency: this is an extremely clever idea. I guess the inspiration came from Node?

Overrides only in the top level module: I understand why only the top level module is allowed to do overrides, but the documentation says *_override is only legal in top-level MODULE.bazel files. Have you considered ignoring *_override statements in non-top-level modules? (probably yes, but I can't find the discussion...)

git_override: why is git special-cased as the only source control system that can supply overrides?

module_ctx.which: why is this particular functionality of the shell exposed in the otherwise very Spartan module_ctx interface? I was about to write that it's trivially implementable using execute(), but that's not the case for Windows and that may be enough of a reason to expose it?

module_ctx.os: this looks like the same mechanism as the one repository_ctx has which predates platforms. Have you talked to the platforms folks to see if they have ideas about an API that would be more coherent with the platforms one they are busy migrating to?

labels in patches=: why are these labels? The documentation says that they have to reference the top-level module so the repository part of a label doesn't make sense here and the label syntax implies that you raise an error when the ":" is at the wrong place (i.e. //a/b:c/d instead of //a:b/c/d or //a/b/c:d) which doesn't look easily implementable, does it?

--

Lukács T. Berki | Software Engineer | lbe...@google.com | 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg, HRB 86891

[Xudong Yang]

+Yun Peng 

Glad you like it, Lukács :D

> Underscore as name component separator: it looks like you settled upon using underscore as the name component separator so that the namespace of module names can be hierarchical.

I don't think we made such a decision. Underscore is just like any other character allowed in module names.

> --ignore_dev_dependency: this is an extremely clever idea. I guess the inspiration came from Node?

I think this was mostly modeled after Cargo's dev dependency thing (Yun can give more context)

> Overrides only in the top level module: I understand why only the top level module is allowed to do overrides, but the documentation says *_override is only legal in top-level MODULE.bazel files. Have you considered ignoring *_override statements in non-top-level modules? (probably yes, but I can't find the discussion...)

Ignoring them was the idea at first, but +Jon Brandvein convinced me that it's better to disallow them altogether, in the case that we eventually allow some form of overrides from transitive dependencies. It's fine to allow something that used to be disallowed, but backwards-incompatible to start giving meaning to something that used to be ignored.

> git_override: why is git special-cased as the only source control system that can supply overrides?

The class of "non-registry overrides" (archive, git, local_path) can be extended fairly easily. Git was just the first one that we put there because it's the most popular one and there's already a git_repository repo rule in bazel_tools. Theoretically we can easily add something like hg_override (provided we get such a repo rule in bazel_tools first).

> module_ctx.which: why is this particular functionality of the shell exposed in the otherwise very Spartan module_ctx interface?

Module extension implementation functions mostly try to run some tools to deduce what repos to generate, so `which` seemed to be a reasonable inclusion. It mostly differs from repository_ctx in that writing into the working directory is discouraged, since content there isn't addressable later on.

> module_ctx.os: this looks like the same mechanism as the one repository_ctx has which predates platforms. Have you talked to the platforms folks to see if they have ideas about an API that would be more coherent with the platforms one they are busy migrating to?

We haven't discussed this recently, and this is somewhat of a chicken-and-egg problem. Bzlmod technically works in a loading-interleaved phase, at which point none of the platform/toolchain resolution thing has happened yet. So it doesn't really make sense for module extension evaluation to depend on things like constraint settings and values.

Last time I chatted to +Jon Brandvein about this, we decided that it may be possible to do a workspace-inside-a-workspace thing if one really cared, and left it at that.

> labels in patches=: why are these labels? The documentation says that they have to reference the top-level module so the repository part of a label doesn't make sense here and the label syntax implies that you raise an error when the ":" is at the wrong place (i.e. //a/b:c/d instead of //a:b/c/d or //a/b/c:d) which doesn't look easily implementable, does it?

This is just because the underlying repo rule http_archive accepts labels for the `patches` argument. I believe the ":" thing is actually checked because we use PackageLookupFunction to find the file. That said, the usage of labels in repo rules / module extensions has always been a bit inelegant since only source file labels can be used, but we didn't deem it bad enough to address.

"  YÁNG Xùdōng (杨旭东)

#  SURNAME Givenname

=  Software Engineer

@  Google Munich

[Yun Peng]

--ignore_dev_dependency: this is an extremely clever idea. I guess the inspiration came from Node?

I wasn't actually aware that Node or Cargo has this feature. It came as a natural way to allow users to test building targets without dev dependencies fetched (even declared), because they won't be available when the project is used as a dependency.

Glad you like this idea ;)

[Lukács T. Berki]

On Fri, Sep 23, 2022 at 2:12 PM Xudong Yang <w...@google.com> wrote:

+Yun Peng 

Glad you like it, Lukács :D

> Underscore as name component separator: it looks like you settled upon using underscore as the name component separator so that the namespace of module names can be hierarchical.

I don't think we made such a decision. Underscore is just like any other character allowed in module names.

Oh well. As I said before, I think this ship has already sailed. If it really becomes necessary to have a hierarchical namespace, we can always allowlist character that is not allowed now.

 

> --ignore_dev_dependency: this is an extremely clever idea. I guess the inspiration came from Node?

I think this was mostly modeled after Cargo's dev dependency thing (Yun can give more context)

> Overrides only in the top level module: I understand why only the top level module is allowed to do overrides, but the documentation says *_override is only legal in top-level MODULE.bazel files. Have you considered ignoring *_override statements in non-top-level modules? (probably yes, but I can't find the discussion...)

Ignoring them was the idea at first, but +Jon Brandvein convinced me that it's better to disallow them altogether, in the case that we eventually allow some form of overrides from transitive dependencies. It's fine to allow something that used to be disallowed, but backwards-incompatible to start giving meaning to something that used to be ignored.

All the power to you! I like making things errors at first. I'm somewhat worried about the proliferation of "this MODULE.bazel file is for when you are using this as a dependency and this another one is for when it's a top-level project", but I guess if you are not seeing that today in the registry, it's less of a problem than I thought it would be.

> git_override: why is git special-cased as the only source control system that can supply overrides?

The class of "non-registry overrides" (archive, git, local_path) can be extended fairly easily. Git was just the first one that we put there because it's the most popular one and there's already a git_repository repo rule in bazel_tools. Theoretically we can easily add something like hg_override (provided we get such a repo rule in bazel_tools first).

It looks like git_override is implemented in Starlark under the hood anyway, so I can convince myself that this is not in fact special-casing git but packaging the code to talk to it.

Trying to channel Ivo here, I think it would be better if one required a load() statement because moving git_repository out of @bazel_tools is in line with his desire to move the Starlark rules out of the Bazel binary, too, but if he's fine with this, I'm fine with it, too.

> module_ctx.which: why is this particular functionality of the shell exposed in the otherwise very Spartan module_ctx interface?

Module extension implementation functions mostly try to run some tools to deduce what repos to generate, so `which` seemed to be a reasonable inclusion. It mostly differs from repository_ctx in that writing into the working directory is discouraged, since content there isn't addressable later on.

> module_ctx.os: this looks like the same mechanism as the one repository_ctx has which predates platforms. Have you talked to the platforms folks to see if they have ideas about an API that would be more coherent with the platforms one they are busy migrating to?

We haven't discussed this recently, and this is somewhat of a chicken-and-egg problem. Bzlmod technically works in a loading-interleaved phase, at which point none of the platform/toolchain resolution thing has happened yet. So it doesn't really make sense for module extension evaluation to depend on things like constraint settings and values.

Last time I chatted to +Jon Brandvein about this, we decided that it may be possible to do a workspace-inside-a-workspace thing if one really cared, and left it at that.

(cc John C)

This looks reasonable and I totally thought that Bazel learned the set of constraints representing the platform it runs on by fetching a special repository and therefore there would be a circular dependency if I had my way. That turns out to be technically correct, but the code that determines it happens to live in Java:

https://cs.opensource.google/bazel/bazel/+/3e927e4cc85a1fb2af1b286891a75669b5eb1a7c:src/main/java/com/google/devtools/build/lib/bazel/repository/LocalConfigPlatformFunction.java

If Bazel determined the platform it's running on by shelling out from a repository, I'd totally agree with that assessment, but as it is, it looks simple to lift the code that generates the list of constraint labels to some place where module_ctx.os() can call it. WDYT? If we want a nice symmetric API between bzlmod and platforms, now is the easiest time.

> labels in patches=: why are these labels? The documentation says that they have to reference the top-level module so the repository part of a label doesn't make sense here and the label syntax implies that you raise an error when the ":" is at the wrong place (i.e. //a/b:c/d instead of //a:b/c/d or //a/b/c:d) which doesn't look easily implementable, does it?

This is just because the underlying repo rule http_archive accepts labels for the `patches` argument. I believe the ":" thing is actually checked because we use PackageLookupFunction to find the file. That said, the usage of labels in repo rules / module extensions has always been a bit inelegant since only source file labels can be used, but we didn't deem it bad enough to address.

All the power to you; as long as you are aware of the fact that now is the easiest time to fix this little wart, please go ahead.

"  YÁNG Xùdōng (杨旭东)

#  SURNAME Givenname

=  Software Engineer

@  Google Munich

On Fri, Sep 23, 2022 at 1:47 PM Lukács T. Berki <lbe...@google.com> wrote:

Hey Xudong,

I finally managed to review the progress of bzlmod before making it generally available and I have to say, it's excellent work; I am not wise enough to tell whether it will work out as a full replacement for WORKSPACE files, but this is about the best attempt I can imagine that's humanly possible so I'll do my best to move the needle along. I especially like the effort that went into making it possible to gradually migrate from WORKSPACE files.

I have a few observations, none showstoppers and quite possibly some of them are only due to my ignorance and probably quite some of them can't be changed without much sweat anyways, but it's better to discuss them now before bzlmod becomes officially supported.

Underscore as name component separator: it looks like you settled upon using underscore as the name component separator so that the namespace of module names can be hierarchical. This is somewhat unfortunate because underscore is used in many places, but I guess it's too late to change now, so meh. There isn't an obviously better choice anyway.

--ignore_dev_dependency: this is an extremely clever idea. I guess the inspiration came from Node?

Overrides only in the top level module: I understand why only the top level module is allowed to do overrides, but the documentation says *_override is only legal in top-level MODULE.bazel files. Have you considered ignoring *_override statements in non-top-level modules? (probably yes, but I can't find the discussion...)

git_override: why is git special-cased as the only source control system that can supply overrides?

module_ctx.which: why is this particular functionality of the shell exposed in the otherwise very Spartan module_ctx interface? I was about to write that it's trivially implementable using execute(), but that's not the case for Windows and that may be enough of a reason to expose it?

module_ctx.os: this looks like the same mechanism as the one repository_ctx has which predates platforms. Have you talked to the platforms folks to see if they have ideas about an API that would be more coherent with the platforms one they are busy migrating to?

labels in patches=: why are these labels? The documentation says that they have to reference the top-level module so the repository part of a label doesn't make sense here and the label syntax implies that you raise an error when the ":" is at the wrong place (i.e. //a/b:c/d instead of //a:b/c/d or //a/b/c:d) which doesn't look easily implementable, does it?

--

Lukács T. Berki | Software Engineer | lbe...@google.com | 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg, HRB 86891

[John Cater]

On Fri, Sep 23, 2022 at 9:43 AM Lukács T. Berki <lbe...@google.com> wrote:

> module_ctx.os: this looks like the same mechanism as the one repository_ctx has which predates platforms. Have you talked to the platforms folks to see if they have ideas about an API that would be more coherent with the platforms one they are busy migrating to?

We haven't discussed this recently, and this is somewhat of a chicken-and-egg problem. Bzlmod technically works in a loading-interleaved phase, at which point none of the platform/toolchain resolution thing has happened yet. So it doesn't really make sense for module extension evaluation to depend on things like constraint settings and values.

Last time I chatted to +Jon Brandvein about this, we decided that it may be possible to do a workspace-inside-a-workspace thing if one really cared, and left it at that.

(cc John C)

This looks reasonable and I totally thought that Bazel learned the set of constraints representing the platform it runs on by fetching a special repository and therefore there would be a circular dependency if I had my way. That turns out to be technically correct, but the code that determines it happens to live in Java:

https://cs.opensource.google/bazel/bazel/+/3e927e4cc85a1fb2af1b286891a75669b5eb1a7c:src/main/java/com/google/devtools/build/lib/bazel/repository/LocalConfigPlatformFunction.java

If Bazel determined the platform it's running on by shelling out from a repository, I'd totally agree with that assessment, but as it is, it looks simple to lift the code that generates the list of constraint labels to some place where module_ctx.os() can call it. WDYT? If we want a nice symmetric API between bzlmod and platforms, now is the easiest time.

If I understand correctly, module_ctx execution always and only happens on the host platform (and can't be done remotely). If this is true, we always know the host platform, and can provide it to module_ctx: it's the value of the `--host_platform` flag, which might be set via a flag but which defaults to `@local_config_platform://host` (and which, as Lukacs points out, is implemented in Java).

The _problem_ there is that for the host platform to mean anything, we need to load a bunch of other packages: `@platforms//:os` and `@platforms//:cpu` at a minimum, and potentially an arbitrary number of user-defined packages (if, for example, a user specifies `--host_platform=@//:host_platform`, Bazel needs to load the  top-level package and recursively everything it loads).

This could very easily lead to weird cyclic dependencies: if a user defines a custom host platform in a package, and that package also depends on a module, and the module needs the host platform (for module_ctx), then we've hit a loop. It'd be easy to detect but hard to explain to users what has gone wrong.

Possible solutions:

1. Something complicated with partial loading of BUILD files so that at least we only have errors for direct cycles, not indirect cycles when a package loads from a module and defines a platform.

2. Don't allow actual platforms in module_ctx, instead give a set of constraints from "blessed" packages like `@platforms//:os` and `@platforms//:cpu`.

3. Keep doing whatever we're doing with module_ctx.os and leave platforms out of it, as platforms are analysis time and modules are load time.

[Lukács T. Berki]

I think this is the simplest and most reasonable approach. If module_ctx.os() returned e.g. "@platforms//:x86_64" as a simple string or a Label, it would already be a little more coherent with the rest of Bazel than what it does now.

  

3. Keep doing whatever we're doing with module_ctx.os and leave platforms out of it, as platforms are analysis time and modules are load time.

[Xudong Yang]

> Trying to channel Ivo here, I think it would be better if one required a load() statement because moving git_repository out of @bazel_tools is in line with his desire to move the Starlark rules out of the Bazel binary, too, but if he's fine with this, I'm fine with it, too.

We can't remove everything from @bazel_tools (for example, http_archive is never going away; neither is the shell runfiles library). So it seems to me that unless the specific need arises, we shouldn't try to move git_repository out.

> If module_ctx.os() returned e.g. "@platforms//:x86_64" as a simple string or a Label, it would already be a little more coherent with the rest of Bazel than what it does now.

We certainly could do that, but that'd be a departure from repository_ctx.os, and I don't want to introduce this discrepancy. Right now module_ctx and repository_ctx are much more similar to each other than to the rest of Bazel, and for good reason.

"  YÁNG Xùdōng (杨旭东)

#  SURNAME Givenname

=  Software Engineer

@  Google Munich

[Lukács T. Berki]

On Thu, Sep 29, 2022 at 7:40 PM Xudong Yang <w...@google.com> wrote:

> Trying to channel Ivo here, I think it would be better if one required a load() statement because moving git_repository out of @bazel_tools is in line with his desire to move the Starlark rules out of the Bazel binary, too, but if he's fine with this, I'm fine with it, too.

We can't remove everything from @bazel_tools (for example, http_archive is never going away; neither is the shell runfiles library)

Ivo, is that so? I was under the impression that:

1. Even the shell runfiles library is subject to the long-term "remove everything from Bazel" goal. 
2. There isn't a fundamental reason why http_archive() should stay there (although we might choose to do so because it's convenient)

 

. So it seems to me that unless the specific need arises, we shouldn't try to move git_repository out. 

...but regardless of the answer to the above, I don't think it's worth spending time on moving git_repository out *right now* because we have bigger fish to try.

 

> If module_ctx.os() returned e.g. "@platforms//:x86_64" as a simple string or a Label, it would already be a little more coherent with the rest of Bazel than what it does now.

We certainly could do that, but that'd be a departure from repository_ctx.os, and I don't want to introduce this discrepancy. Right now module_ctx and repository_ctx are much more similar to each other than to the rest of Bazel, and for good reason.

What's that good reason? Ease of migration?

 

I thought bzlmod would be a good natural "break point" to introduce some consistency between how repository rules and the rest of Bazel think about operating systems and platforms, but if you say that you don't want to put this extra roadblock in the way of people trying to use bzlmod, I could get behind that; it's a big and useful enough change already and I don't think there is any huge benefit of using the labels instead of the current ad-hoc strings, "only" the extra bit of consistency in the APIs Bazel offers.

[Yun Peng]

1. Even the shell runfiles library is subject to the long-term "remove everything from Bazel" goal. 

The shell runfiles library needs to stay in bazel_tools, because the boilerplate code to "import" the runfiles library expects a stable repo name. With Bzlmod, bazel_tools is the only one that has a stable canonical repo name.

 

1. There isn't a fundamental reason why http_archive() should stay there (although we might choose to do so because it's convenient)

We rely on http_archive to fetch Bazel module, if http_archive is moved to a Bazel module, then there is going to be a chicken-and-egg problem.

 

 

. So it seems to me that unless the specific need arises, we shouldn't try to move git_repository out. 

...but regardless of the answer to the above, I don't think it's worth spending time on moving git_repository out *right now* because we have bigger fish to try.

Agree.

[Lukács T. Berki]

On Fri, Sep 30, 2022 at 10:43 AM Yun Peng <pcl...@google.com> wrote:

1. Even the shell runfiles library is subject to the long-term "remove everything from Bazel" goal. 

The shell runfiles library needs to stay in bazel_tools, because the boilerplate code to "import" the runfiles library expects a stable repo name. With Bzlmod, bazel_tools is the only one that has a stable canonical repo name.

Oof. On the surface, it looks like Python (and any other interpreted language) would also be affected by this and I suppose Python is not because it tries every top-level directory in the runfiles tree when importing?

[Yun Peng]

Oof. On the surface, it looks like Python (and any other interpreted language) would also be affected by this and I suppose Python is not because it tries every top-level directory in the runfiles tree when importing?

Yeah, Python is kind of the opposite, you can do both `import <repo_name>.tools.runfiles` or `import tools.runfiles`. The first one is Bazel specific and not the way of doing things in the python world, but the later one may shadow the "tools" package from other repos (see #7051, #2636). 

For most python projects, there is usually a top level folder that matches its unique package name (tensorflow, numpy, glfags) to avoid this conflict. I think Bazel Python projects should also adopt this convention to avoid this problem (e.g. rules_python, which provides the python runfiles library).

There is a recently discussion on this: https://groups.google.com/a/google.com/g/build-semantics/c/VwHB9DLbpWQ/m/UpEo9P2KBgAJ

--
You received this message because you are subscribed to the Google Groups "bazel-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bazel-dev+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-dev/CAOu%2B0LVi2bfqdEC_knjWddiwX7qpowhok%2BkQZT82qr67XUmyhA%40mail.gmail.com.

[Alex Eagle]

I think the runfiles library should simply be published to Pypi like any other Python package, rather than being distributed inside a Bazel module. That's what we do for JS: https://npmjs.com/@bazel/runfiles

Then it doesn't require any special import path handling.

The only downside is possible version skew between the runfiles package and the rules_python version, however there's no coupling between them so I don't think that matters.

-Alex