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

Export bzl files with bzl_library to be able to use stardoc #1092

Closed
wants to merge 4 commits into from
Closed

Conversation

ngeor
Copy link
Contributor

@ngeor ngeor commented Aug 12, 2021

Stardoc is a documentation tool for Bazel rules/macros/etc. We need to group the bzl files of bazel-gazelle into bzl_library targets so that stardoc can pick them up.

What type of PR is this?

Uncomment one line below and remove others.

Other

What package or component does this PR mostly affect?

For example:

language/go
cmd/gazelle
go_repository
all

What does this PR do? Why is it needed?

Which issues(s) does this PR fix?

Fixes #

Other notes for review

Stardoc is a documentation tool for Bazel rules/macros/etc. We need to group the bzl files of bazel-gazelle into bzl_library targets so that stardoc can pick them up.
@achew22
Copy link
Member

achew22 commented Aug 16, 2021

@ngeor, sorry for the slow response. I took the weekend off of computers.

This looks like a good thing to add to the repo. Given that Gazelle has a plugin for generating these targets what would you think about either generating them with, or generating them the same as the generator would have?

@ngeor
Copy link
Contributor Author

ngeor commented Aug 17, 2021

That sounds like a great idea. If you have any quick pointers on how to do that, it would be great. I searched a bit for docs or examples but I wasn't very lucky.

@achew22
Copy link
Member

achew22 commented Aug 17, 2021

Absolutely! I'm terribly sorry about my lack of clarity on there. The testdata directory in there contains a lot of examples of how it is generated. The directories are testcases where the BUILD.out file is what the generator would generate if that were the case.

For example:

defaultvisibility
├── BUILD.in
├── BUILD.out
├── WORKSPACE
├── foo.bzl
└── nested
    └── dir
        ├── BUILD.in
        ├── BUILD.out
        └── bar.bzl

So,

$ cat BUILD.out
load("@bazel_skylib//:bzl_library.bzl", "bzl_library")

bzl_library(
    name = "foo",
    srcs = ["foo.bzl"],
    visibility = ["//visibility:public"],
    deps = ["//nested/dir:bar"],
)
$ cat nested/dir/BUILD.out
load("@bazel_skylib//:bzl_library.bzl", "bzl_library")

package(default_visibility = ["//:__pkg__"])

bzl_library(
    name = "bar",
    srcs = ["bar.bzl"],
)

So the objective is to create a bzl_library for each .bzl file and name it the same as the file without the extension.

@ngeor
Copy link
Contributor Author

ngeor commented Aug 18, 2021

So, I'm failing so far, maybe I'm missing something obvious... you can see what I did in the most recent commit.

  • I am trying to upgrade bazel-skylib to the latest tag
  • Then, I'm adding the @bazel-skylib/gazelle/bzl language to the list of languages

But, building this fails, complaining that gazelle/bzl doesn't have a BUILD file

gitpod /workspace/bazel-gazelle $ bazel build //...
ERROR: /workspace/bazel-gazelle/BUILD.bazel:19:15: no such package '@bazel_skylib//gazelle/bzl': BUILD file not found in directory 'gazelle/bzl' of external repository @bazel_skylib. Add a BUILD file to a directory to mark it as a package. and referenced by '//:gazelle_local'
ERROR: Analysis of target '//:gazelle_local' failed; build aborted: Analysis failed
INFO: Elapsed time: 0.993s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (38 packages loaded, 726 targets configured)

My idea was to temporarily add this language to gazelle and then run it with bazel run //:gazelle so it auto-fixes the BUILD files for me.

deps.bzl Outdated
@@ -79,7 +79,7 @@ def gazelle_dependencies(
_tools_git_repository,
name = "bazel_skylib",
remote = "https://github.com/bazelbuild/bazel-skylib",
commit = "3fea8cb680f4a53a129f7ebace1a5a4d1e035914", # 0.5.0 as of 2018-11-01
tag = "1.0.3"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess the 1.0.3 tag (from > a year ago) doesn't contain the Gazelle plugin at all. I think you need to grab a more recent commit like df3c9e2735f02a7fe8cd80db4db00fec8e13d25f

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it still fails with that commit, but also I see the Gazelle plugin existed even in the 1.0.3 tag, i.e. https://github.com/bazelbuild/bazel-skylib/blob/1.0.3/gazelle/bzl/BUILD

I'm a bit new to Bazel so maybe I'm doing something completely wrong.

Is there some possibility that this is causing a circular dependency? You are depending on skylib, they are depending on gazelle, maybe this is Bazel's way of telling me that this isn't going to work. After all, as you're already depending on skylib, why didn't you add support for bzl language inside gazelle to begin with? Maybe someone tried and failed because of this?

Thanks for your help and patience btw :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some possibility that this is causing a circular dependency? You are depending on skylib, they are depending on gazelle, maybe this is Bazel's way of telling me that this isn't going to work. After all, as you're already depending on skylib, why didn't you add support for bzl language inside gazelle to begin with? Maybe someone tried and failed because of this?

It's possible but I don't think so (see below). When Bazel has circular deps it prints out a VERY specific error that says circular dependency detected and then it renders a graph that shows you the loop that you need to resolve.

Thanks for your help and patience btw :)

Truly, you're the one contributing here, you deserve all the praise. Thanks for helping out!

I think the issue is that the version of skylib in this PR doesn't have gazelle. I validated this by cloning your repo and querying for all the targets that live in @bazel_skylib//... (basically everything in there).

$ bazel query @bazel_skylib//...
@bazel_skylib//toolchains/unittest:cmd_toolchain
@bazel_skylib//toolchains/unittest:cmd
@bazel_skylib//toolchains/unittest:bash_toolchain
@bazel_skylib//toolchains/unittest:toolchain_type
@bazel_skylib//toolchains/unittest:bash
@bazel_skylib//rules/private:maprule_util
@bazel_skylib//rules:write_file
@bazel_skylib//rules/private:write_file_private
@bazel_skylib//rules:select_file
@bazel_skylib//rules:run_binary
@bazel_skylib//rules:native_binary
@bazel_skylib//rules:diff_test
@bazel_skylib//rules:copy_file
@bazel_skylib//rules/private:copy_file_private
@bazel_skylib//rules:common_settings
@bazel_skylib//rules:build_test
@bazel_skylib//rules:analysis_test
@bazel_skylib//lib:old_sets
@bazel_skylib//:workspace
@bazel_skylib//:version
@bazel_skylib//:test_deps
@bazel_skylib//toolchains/unittest:test_deps
@bazel_skylib//rules:test_deps
@bazel_skylib//lib:test_deps
@bazel_skylib//:lib
@bazel_skylib//lib:versions
@bazel_skylib//lib:unittest
@bazel_skylib//lib:types
@bazel_skylib//lib:structs
@bazel_skylib//lib:shell
@bazel_skylib//lib:sets
@bazel_skylib//lib:selects
@bazel_skylib//lib:paths
@bazel_skylib//lib:partial
@bazel_skylib//lib:new_sets
@bazel_skylib//lib:dicts
@bazel_skylib//lib:collections
@bazel_skylib//:distribution
@bazel_skylib//toolchains/unittest:distribution
@bazel_skylib//rules/private:distribution
@bazel_skylib//rules:distribution
@bazel_skylib//lib:distribution
@bazel_skylib//:bzl_library
@bazel_skylib//:bins
@bazel_skylib//rules:bins

Looking that makes me very suspicious because it looks just like the directory listing for a long time ago. With a little bit more digging I was able to confirm the directory listing is identical to what you get when you grab an old skylib.

Turns out that WORKSPACEs suck and have all kinds of footguns. For example, if you re-register an external workspace that differs from the one that's already been defined it silently ignores your request. I believe that's what is happening here. I can't find the transitive dependency that's wrong, but when I add to the head of the WORKSPACE file right after the workspace(name = "bazel_gazelle") line:

load(
    "@bazel_tools//tools/build_defs/repo:git.bzl",
    "git_repository",
)

git_repository(
    name = "bazel_skylib",
    commit = "df3c9e2735f02a7fe8cd80db4db00fec8e13d25f",  # `master` as of 2021-08-19
    remote = "https://github.com/bazelbuild/bazel-skylib",
)

Everything works immediately. Would you be willing to manually make that change to WORKSPACE and run Gazelle?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I would expect Bazel to complain about this... thanks for figuring it out!

So I guess now the question is should I leave the skylib plugin permanently in gazelle or revert it?

@achew22
Copy link
Member

achew22 commented Aug 19, 2021

This has kind of been a long running thing for me. I added you as a co-author on #760, would you be willing to tell the robot on the PR over there that you're cool with this and land that other PR instead of this one?

@ngeor
Copy link
Contributor Author

ngeor commented Aug 20, 2021

Yeah definitely. Let's close this one then.

Closing in favor of #760

@ngeor ngeor closed this Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants