Add the gazelle plugin to the distribution #400

shs96c · 2022-10-06T13:59:45Z

To avoid everyone needing to take a dep on rules_go, we do the following:

Regular Bazel users need to load bazel_skylib_gazelle_plugin_workspace and call that, and then bazel_skylib_gazelle_plugin_setup
bzlmod users need do nothing, but we now include the rules_go dep in the MODULE.bazel shipped in the release. This is fine, because bzlmod will lazily load dependencies.

MODULE.bazel

shs96c · 2022-10-14T12:50:05Z

Is there anything else I need to do?

fmeum

I think you may have removed all reviewers with write access to this repo :-)

fmeum · 2022-10-18T09:52:02Z

gazelle/bzl/BUILD

@@ -51,3 +51,25 @@ gazelle(
    name = "gazelle",
    gazelle = ":gazelle-skylib",
 )
+
+# The files needed for distribution


Would adding allow_empty to the glob be an alternative? It wouldnt' be error-prone in this context.

According to the docs, allow_empty defaults to True, but I've made it explicit.

fmeum · 2022-10-18T09:55:35Z

gazelle_setup.bzl

+
+def bazel_skylib_gazelle_plugin_setup():
+    go_rules_dependencies()
+    go_register_toolchains(version = "1.17.1")


Have you tried calling this macro from a WORKSPACE that already sets up a Go toolchain somewhere else? You might have to make running this configurable via a parameter.

If possible, also bump the version to 1.18.7 as 1.17 is out of support. 1.19 is available, but requires a relatively recent version of rules_go.

Done and done.

To avoid everyone needing to take a dep on `rules_go`, we do the following: 1. Regular Bazel users need to load `bazel_skylib_gazelle_plugin_workspace` and call that, and then `bazel_skylib_gazelle_plugin_setup` 2. `bzlmod` users need do nothing, but we now include the `rules_go` dep in the `MODULE.bazel` shipped in the release. This is fine, because `bzlmod` will lazily load dependencies.

shs96c · 2022-10-18T13:57:06Z

@fmeum I thought you could commit to this repo :) Oops. Will add an owner as a reviewer!

shs96c · 2022-10-18T13:59:45Z

Apparently, I can't add reviewers now in the GH UI, but it looks like @Wyverald can merge changes. Pinging them in this comment :)

Wyverald · 2022-10-18T14:37:18Z

Have you tried out the approach we discussed in #250 where the part using gazelle is separated out into its own module? I still think it's rather intrusive to have rules_go in basically everyone's dependency graph.

shs96c · 2022-10-18T16:14:25Z

As I understand it, bzlmod will only lazily load the dependencies, so it should make any difference to the time it takes to build a project. Having rules_go in the dependency graph will mostly be a no-op. I don't think users will be in the habit of expecting more than one module per repo, so the user experience of this approach is "lower friction" for bzlmod users.

For people not using bzlmod, they need to actively pull in the rules_go dep by calling gazelle_setup: it's not part of the default workflow.

fmeum · 2022-10-18T17:50:47Z

I obviously don't like saying that, but unfortunately having a transitive Bzlmod dep on rules_go isn't really a no-op (yet):

Since we are still in a phase of rapid iteration on rules_go's Bzlmod interface, it emits warnings like the following when depended on:

DEBUG: gazelle@0.26.0/MODULE.bazel:6:6: WARNING: The bazel_gazelle Bazel module is still highly experimental and subject to change at any time. Only use it to try out bzlmod for now.
DEBUG: rules_go@0.33.0/MODULE.bazel:7:6: WARNING: The rules_go Bazel module is still highly experimental and subject to change at any time. Only use it to try out bzlmod for now.

We will try to stabilize this shortly after the Bazel 6 release, but I can't promise anything.

I am still working on making the toolchain fetching in rules_go lazy. Currently, it does perform some network I/O if it is depended on even if you don't build Go targets.

At least before both these issues have been fixed introducing a rules_go dependency on a core module such as skylib won't be a good user experience. It should be pretty simple to migrate users off of a separate module though after these limitations have been addressed.

Wyverald · 2022-10-24T12:44:47Z

As I understand it, bzlmod will only lazily load the dependencies, so it should make any difference to the time it takes to build a project.

That's true, but as Fabian also pointed out, bringing in other modules sometimes does fetch them eagerly, particularly when toolchains are registered in them. Besides, adding dependencies has other costs -- version resolution takes more time and includes more noise, and a "vendor mode" setup ("download everything for me so that I can work offline") will fetch much more random stuff.

I don't think users will be in the habit of expecting more than one module per repo, [...]

If by "repo" here you mean "git repo" instead of "Bazel repo", we need to normalize the expectation of multiple modules hosted in the same git repository. That would be the only sane way to avoid an explosion of the dependency graph.

shs96c · 2022-11-03T17:31:17Z

@Wyverald updated to be a two module, one git repo setup.

shs96c · 2022-11-03T18:16:55Z

The build is failing with last_green because a target in rules_pkg isn't visible:

ERROR: /workdir/distribution/BUILD:17:8: in pkg_tar_impl rule //distribution:bazel-skylib-1.3.0: target '@rules_pkg//private:private_stamp_detect' is not visible from target '//distribution:bazel-skylib-1.3.0'. Check the visibility declaration of the former target if you think the dependency is legitimate
ERROR: /workdir/distribution/BUILD:17:8: Analysis of target '//distribution:bazel-skylib-1.3.0' failed

With last_rc and Bazel 5.3.2, this doesn't appear to be a problem.

fmeum · 2022-11-03T18:17:45Z

@shs96c HEAD broke downstream projects, should be fixed in about an hour thanks to bazelbuild/bazel@7dcf0c3.

… match the ones in gazelle

Wyverald

nice start!

Wyverald · 2022-11-03T18:33:21Z

gazelle/MODULE.bazel

@@ -0,0 +1,24 @@
+bazel_dep(name = "bazel_skylib", version = "1.3.0")


add a module(name="skylib_gazelle", version="...") declaration here?

Wyverald · 2022-11-03T18:33:56Z

MODULE.bazel

@@ -14,8 +14,8 @@ bazel_dep(name = "platforms", version = "0.0.4")
 ### INTERNAL ONLY - lines after this are not included in the release packaging.

 # Gazelle extension is experimental
-bazel_dep(name = "rules_go", repo_name = "io_bazel_rules_go", version = "0.33.0")
-bazel_dep(name = "gazelle", repo_name = "bazel_gazelle", version = "0.26.0")
+bazel_dep(name = "rules_go", repo_name = "io_bazel_rules_go", version = "0.35.0")


is the plan to remove these?

I can try removing them and see what breaks....

…//... works as expected

Wyverald · 2022-11-04T13:52:30Z

WORKSPACE

@@ -69,17 +69,5 @@ register_unittest_toolchains()
 # github.com/bazelbuild/rules_go is available from io_bazel_rules_go and it
 # doesn't need to duplicatively fetch it.
 # gazelle:repository go_repository name=io_bazel_rules_go importpath=github.com/bazelbuild/rules_go


I'm not familiar with gazelle repository hints, do these need to be moved to gazelee/WORKSPACE?

I've moved them, and everything still seems to build. Apparently, it's fine to move them.

Wyverald

all looks good except for the question about the gazelle hints left over in the WORKSPACE file. thanks!

cgrindel · 2022-11-08T23:27:28Z

@shs96c Will we be adding instructions on how to load the @bazel_skylib_gazelle_plugin repository into a workspace?

Add the gazelle plugin to the distribution (bazelbuild#400)

After #400, the gazelle plugin has been cleanly separated out into its own bazel workspace, which will soon finally allow us to mark it stable. But this means: * we need to change our bazelci config to explicitly build and test it, since `bazel build //...` no longer includes the plugin; * we need to add proper distribution rules for it; * we need to update release instructions, since now we will have two distribution tarballs

shs96c requested review from achew22, brandjon, tetromino, comius, hvadehra and c-mita as code owners October 6, 2022 13:59

shs96c mentioned this pull request Oct 6, 2022

Automatic generation of skylark_library targets in BUILD files #250

Closed

fmeum reviewed Oct 6, 2022

View reviewed changes

MODULE.bazel Show resolved Hide resolved

shs96c requested review from fmeum and nickgooding and removed request for achew22, tetromino, brandjon, comius, hvadehra, c-mita and fmeum October 18, 2022 09:48

fmeum approved these changes Oct 18, 2022

View reviewed changes

shs96c added 6 commits October 18, 2022 14:48

Run buildifier

4f269cd

Add docstring to gazelle_setup.bzl

87394f2

Move "internal only" marker

1ed8cad

Make @com_github_bazelbuild_buildtools visible in the deployed module

e96655b

Respond to review comments

962eebd

shs96c force-pushed the gazelle-macros branch from 4d42138 to 962eebd Compare October 18, 2022 13:56

Move plugin to a nested workspace

242167e

Run buildifier

b5938b5

shs96c added 2 commits November 3, 2022 18:18

Restore the module declaration from main

4bf6c88

Bump versions of rules_go and gazelle used in the top-level module to…

1d56cbe

… match the ones in gazelle

Wyverald reviewed Nov 3, 2022

View reviewed changes

shs96c added 2 commits November 4, 2022 13:08

Respond to review comments and add gazelle to ignored directories so …

d3acf47

…//... works as expected

Add missing descriptor for stardoc

4a5457e

Wyverald reviewed Nov 4, 2022

View reviewed changes

Wyverald approved these changes Nov 4, 2022

View reviewed changes

Move gazelle hints into workspace files

73bf9bf

Wyverald approved these changes Nov 7, 2022

View reviewed changes

Wyverald merged commit 60abca8 into bazelbuild:main Nov 7, 2022

shs96c mentioned this pull request Nov 8, 2022

Add gazelle plugin to BCR #410

Closed

leovitch added a commit to leovitch/bazel-skylib that referenced this pull request Nov 15, 2022

Merge pull request #1 from bazelbuild/main

1979599

Add the gazelle plugin to the distribution (bazelbuild#400)

This was referenced Jan 17, 2023

Move gazelle plugin into a new git repo #423

Closed

Add gazelle plugin to CI and distribution mechanism #424

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the gazelle plugin to the distribution #400

Add the gazelle plugin to the distribution #400

shs96c commented Oct 6, 2022

shs96c commented Oct 14, 2022

fmeum left a comment

fmeum Oct 18, 2022

shs96c Oct 18, 2022

fmeum Oct 18, 2022

shs96c Oct 18, 2022

shs96c commented Oct 18, 2022

shs96c commented Oct 18, 2022

Wyverald commented Oct 18, 2022

shs96c commented Oct 18, 2022

fmeum commented Oct 18, 2022

Wyverald commented Oct 24, 2022

shs96c commented Nov 3, 2022

shs96c commented Nov 3, 2022

fmeum commented Nov 3, 2022

Wyverald left a comment

Wyverald Nov 3, 2022

shs96c Nov 4, 2022

Wyverald Nov 3, 2022

shs96c Nov 4, 2022

shs96c Nov 4, 2022

Wyverald Nov 4, 2022

shs96c Nov 7, 2022

Wyverald left a comment

cgrindel commented Nov 8, 2022

		@@ -0,0 +1,24 @@
		bazel_dep(name = "bazel_skylib", version = "1.3.0")

Add the gazelle plugin to the distribution #400

Add the gazelle plugin to the distribution #400

Conversation

shs96c commented Oct 6, 2022

shs96c commented Oct 14, 2022

fmeum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shs96c commented Oct 18, 2022

shs96c commented Oct 18, 2022

Wyverald commented Oct 18, 2022

shs96c commented Oct 18, 2022

fmeum commented Oct 18, 2022

Wyverald commented Oct 24, 2022

shs96c commented Nov 3, 2022

shs96c commented Nov 3, 2022

fmeum commented Nov 3, 2022

Wyverald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wyverald left a comment

Choose a reason for hiding this comment

cgrindel commented Nov 8, 2022