Basic bzlmod setup #3047

fmeum · 2022-01-17T10:40:09Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This PR adds a MODULE.bazel that makes rules_go usable as a bzlmod module for basic use cases.

The new module definition is verified by a test module that will be used in the presubmit tests of the Bazel Central Registry (BCR).

The following features require more thought and/or work and are not yet supported:

SDK rules other than go_host_sdk and go_download_sdk.
non-no-op nogo
go_proto_library

Which issues(s) does this PR fix?

Work towards #3020.

Other notes for review

Based on #3046, so please review that one first.

fmeum · 2022-01-17T11:20:25Z

This is essentially ready for review. The CI fails because the test machines have no Go installed and I chose to use the host SDK as the default toolchain for the new module. Of course we can change that to a hermetic toolchain and/or install Go in the CI.

@achew22 @robfig What is your take on this? Bazel modules are generally expected to be "batteries included" in the sense that they provide a reasonable default toolchain that can then be overriden by the user. In the case of Go, using the host SDK sounds reasonable and would also be in line with how C++ and Java behave with Bazel. However, that would require making a Go SDK available in the CI.

fmeum · 2022-01-17T15:24:02Z

Sorry, I misunderstood the CI failure. Bazel of course has to load the repositories of each toolchain, even if it doesn't end up being selected, so the failure would occur for every user without a local Go SDK. I will look into making this a soft fail, but would already appreciate your feedback on the rest of the PR.

fmeum · 2022-01-20T10:41:19Z

I have put some more thought into how toolchain selection could be improved with bzlmod. I will try to get some feedback on these in the bzlmod community before I continue here.

ar3s3ru · 2022-02-24T15:59:16Z

Hey there @fmeum! Any updates on this PR?
Would be awesome to see rules_go support Bzlmod

fmeum · 2022-02-24T16:02:52Z

I haven't continued working on it since rules_go without gazelle wouldn't be that useful and turning gazelle into a bzlmod module would most likely require more progress on bazelbuild/bazel#14590 and bazelbuild/bazel#14580. I will revisit this PR when things have improved.

ar3s3ru · 2022-02-25T14:13:27Z

@fmeum is it really necessary to turn gazelle into a bzlmod module for this PR to be merged?
AFAIK the dependency is gazelle -> rules_go, not the other way around.

What I can see from the failing CI is that the code is expecting a toolchain to be installed in the host machine by default, but we could package a default toolchain instead and make it hermetic, no?

Can't gazelle be used still within WORKSPACE while these blocking issues are resolved?

fmeum · 2022-05-04T15:36:49Z

@ar3s3ru It is not strictly necessary to turn gazelle into a bzlmod module - but without gazelle, rules_go probably wouldn't be that useful. In particular, go_proto_library wouldn't work without the supporting go repositories. While it would be possible to include these with the patched in BUILD files, the proper way to make this work would indeed be to bzlmodify gazelle, provide a go_repository-like module extension and let rules_go load these repositories in this way.

I would leave the decision to the maintainers whether having an incomplete version of rules_go as a bzlmod module is something they would see as a net positive (rather than just added complexity). If so, I could quickly fix up the PR and get it merged. @achew22 @linzhp What are your thoughts?

achew22 · 2022-05-04T16:34:28Z

Personally I agree with @fmeum. gazelle provides go_repository which is how third-party go code would be loaded into the build graph. I try VERY hard to not add any third-party dependencies on a project, but I'm not sure I've ever made it to production without one. rules_go is, sadly, not very useful without gazelle.

achew22 · 2022-06-07T16:23:08Z

I'm happy to merge this, but what is the story for 3p dependencies declared by rules_go consumers?

fmeum · 2022-06-07T16:55:42Z

@achew22 My current plan is as follows:

Get rules_go into the BCR based on this PR. The BCR is still in beta, so we can make changes at any time - there is no need to commit to an API yet.
Work on a module extension provided by Gazelle that allows modules to declare their go.mod-style (no http/git/forks/... for starters) dependencies in a way that works well with transitive dependencies. I have a rough prototype of this.
Use this module extension in rules_go (cyclic dependencies between modules are permitted) to get the Go modules required for protos to work. That will be the first real test case for these two modules.

After these basics have been found to work, we should start proper discussions on the APIs and features we want to support. I just think it's too early for extended design discussions until there is at least one ruleset handling transitive dependencies in the BCR - currently there are none.

achew22 · 2022-06-07T20:39:00Z

Are we dependent on bzlmod being experimental to note that we are going to make backward incompatible changes? Is there any way to put up some blinking hazard lights to notify anyone who may stumble into it that it's definitely not ready for prime time yet?

fmeum · 2022-06-08T06:40:26Z

Are we dependent on bzlmod being experimental to note that we are going to make backward incompatible changes? Is there any way to put up some blinking hazard lights to notify anyone who may stumble into it that it's definitely not ready for prime time yet?

That's a good point. I added https://github.com/bazelbuild/rules_go/pull/3047/files#diff-6136fc12446089c3db7360e923203dd114b6a1466252e71667c6791c20fe6bdcR10. Is is scary enough? If so, you can merge, I will not make further changes.

With the repository mappings used by bzlmod, the Label(...).workspace_name construct always returns the canonical, post-repo mapping repo name, which can't be used to construct a valid label. Until bazelbuild/bazel#15593 has been fixed, we have to hardcode the non-canonical repo name here.

The existing repository macros can be reused for the module setup with only very few fully backwards compatible changes: * The macros wrapping the Go SDK repository rules should make toolchain registration optional. With bzlmod, toolchains are registered with the toolchains_to_register attribute of the module function instead. * The repository macro should not use _maybe with bzlmod. Instead, repos loaded by module extensions are given globally unique internal names automatically.

achew22 · 2022-06-09T03:37:10Z

bcr_tests/MODULE.bazel

+
+bazel_dep(name = "io_bazel_rules_go", version = "", repo_name = "my_rules_go")
+local_path_override(
+    module_name = "io_bazel_rules_go",


I thought it was going to show up in bzlmod as just rules_go?

It will, but only via patches applied on top of this PR.

I initially tried to make rules_go completely repo name agnostic in a backwards compatible way, but that doesn't seem possible: There are a number of places that require absolute labels with Bazel 4 and other places (see below) where naive absolute labels don't work with bzlmod. I can try again after Bazel 5 has become the required minimum version.

IMO the best way forward will be to keep the number of io_bazel_rules_go mentions low enough that the patch that makes it available as rules_go in the BCR is easy to maintain. New users relying on modules get to choose their repo name freely, all others probably don't want to go through the hassles of renaming everything. When WORKSPACE has finally been deprecated, we can rename everything without and get rid of the patch.

What do you think?

MODULE.bazel

bcr_tests/.bazelrc

achew22 · 2022-06-09T03:41:01Z

go/private/sdk.bzl

@@ -233,7 +237,7 @@ def _sdk_build_file(ctx, platform):
            "{goos}": goos,
            "{goarch}": goarch,
            "{exe}": ".exe" if goos == "windows" else "",
-            "{rules_go_repo_name}": Label("//go/private:BUILD.sdk.bazel").workspace_name,


I think the logic behind this was that it would resolve to io_bazel_rules_go if you imported it as such, but if people started to import as rules_go they would get that. Does this force us to use io_bazel_rules_go going forward?

This is a pattern that currently breaks with bzlmod: With repo mappings, which it uses, the value of workspace_name will be the mapped repo name, which can't be used to form a valid label. This is where bazelbuild/bazel#15593 would help.

As a result, this line either needs to be replaced on master or via a patch in the BCR. I opted for the former as there are other blockers to a non-bzlmod repo name agnostic rules_go (see my other comment).

This commit adds a MODULE.bazel that makes rules_go usable as a bzlmod module for basic use cases. The new module definition is verified by a test module that will be used in the presubmit tests of the Bazel Central Registry (BCR). The following features require more thought and/or work and are not yet supported: * SDK rules other than go_host_sdk and go_download_sdk. * non-no-op nogo * go_proto_library

achew22

LGTM % 1

MODULE.bazel

fmeum force-pushed the basic-bzlmod branch 2 times, most recently from d3b5de8 to 35fc106 Compare January 17, 2022 11:02

fmeum marked this pull request as ready for review January 17, 2022 11:18

fmeum force-pushed the basic-bzlmod branch from 35fc106 to ada28cb Compare June 6, 2022 21:07

fmeum force-pushed the basic-bzlmod branch from ada28cb to 47ed8b1 Compare June 7, 2022 18:36

fmeum force-pushed the basic-bzlmod branch from 47ed8b1 to 08a00df Compare June 8, 2022 06:39

fmeum force-pushed the basic-bzlmod branch from 08a00df to ef032fe Compare June 8, 2022 07:27

fmeum added 2 commits June 8, 2022 09:31

Remove remaining repo-absolute labels in load statements

8eb8468

fmeum force-pushed the basic-bzlmod branch 2 times, most recently from 40b563a to a76fcc5 Compare June 8, 2022 11:12

fmeum mentioned this pull request Jun 8, 2022

bzlmod prototype bazel-contrib/bazel-gazelle#1266

Merged

achew22 reviewed Jun 9, 2022

View reviewed changes

fmeum force-pushed the basic-bzlmod branch 2 times, most recently from dc2cc96 to 22102bd Compare June 9, 2022 08:23

fmeum force-pushed the basic-bzlmod branch from 22102bd to 92a17aa Compare June 9, 2022 08:26

sluongng mentioned this pull request Jun 13, 2022

doc: staticcheck README sluongng/nogo-analyzer#14

Merged

achew22 approved these changes Jul 1, 2022

View reviewed changes

MODULE.bazel Show resolved Hide resolved

fmeum merged commit e80bb2c into bazel-contrib:master Jul 1, 2022

fmeum deleted the basic-bzlmod branch July 1, 2022 17:09

This was referenced Oct 11, 2023

chore(deps): update dependency io_bazel_rules_go to v0.42.0 - autoclosed chickenandpork/rules_jq#8

Closed

chore(deps): update dependency io_bazel_rules_go to v0.44.2 locol23/monorepo-boilerplate#86

Merged

renovate bot mentioned this pull request Oct 21, 2023

chore(deps): update dependency io_bazel_rules_go to v0.42.0 danmx/bazel-tools#44

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic bzlmod setup #3047

Basic bzlmod setup #3047

fmeum commented Jan 17, 2022 •

edited

Loading

fmeum commented Jan 17, 2022

fmeum commented Jan 17, 2022

fmeum commented Jan 20, 2022

ar3s3ru commented Feb 24, 2022

fmeum commented Feb 24, 2022

ar3s3ru commented Feb 25, 2022

fmeum commented May 4, 2022

achew22 commented May 4, 2022

achew22 commented Jun 7, 2022

fmeum commented Jun 7, 2022

achew22 commented Jun 7, 2022

fmeum commented Jun 8, 2022 •

edited

Loading

achew22 Jun 9, 2022

fmeum Jun 9, 2022

achew22 Jun 9, 2022

fmeum Jun 9, 2022

achew22 left a comment

Basic bzlmod setup #3047

Basic bzlmod setup #3047

Conversation

fmeum commented Jan 17, 2022 • edited Loading

fmeum commented Jan 17, 2022

fmeum commented Jan 17, 2022

fmeum commented Jan 20, 2022

ar3s3ru commented Feb 24, 2022

fmeum commented Feb 24, 2022

ar3s3ru commented Feb 25, 2022

fmeum commented May 4, 2022

achew22 commented May 4, 2022

achew22 commented Jun 7, 2022

fmeum commented Jun 7, 2022

achew22 commented Jun 7, 2022

fmeum commented Jun 8, 2022 • edited Loading

achew22 Jun 9, 2022

Choose a reason for hiding this comment

fmeum Jun 9, 2022

Choose a reason for hiding this comment

achew22 Jun 9, 2022

Choose a reason for hiding this comment

fmeum Jun 9, 2022

Choose a reason for hiding this comment

achew22 left a comment

Choose a reason for hiding this comment

fmeum commented Jan 17, 2022 •

edited

Loading

fmeum commented Jun 8, 2022 •

edited

Loading