Create bzl_library entries using gazelle-skylark #2621

achew22 · 2020-08-24T18:59:57Z

There are a few minor manual modifications that needed to be performed.
For example, some .bzl files depend on .bzl files that are generated
outside of this repo and do not presently exist. @bazel_tools is a
great example of this.

Additionally this effort has revealed that bazel gazelle has an explicit
dependency on the private parts of rules_go in
internal/gazelle_binary.bzl on line 21 where it imports from
go/private:rules/transitions.bzl which is no longer the path to import
from once I created the BUILD file in go/private/rules.

As a temporary fix for this, I'm included a patch that changes that to
point to the new path. I will provide a follow up patch once rules_go
has been released to fix it properly in the gazelle repo and then remove
the patch from rules_go.

Fixed: #2619
Bug: bazelbuild/bazel-skylib#250
Bug: bazel-contrib/bazel-gazelle#803

What type of PR is this?

Uncomment one line below and remove others.

Feature

What does this PR do? Why is it needed?

Adds a tree of bzl_library targets (generated by Gazelle with only small hand modifications for a few places where their deps don't actually exist).

Which issues(s) does this PR fix?

Fixes #2619

Other notes for review

This was great feedback for a couple of features that need to be added to the gazelle generator for bzl_library

achew22 · 2020-08-24T21:08:38Z

@aherrmann, I would like to add you as a Co-authored-by on the PR before merging, is that okay with you?

jayconrod

Some initial comments. I'm pretty swamped this week though, so I won't have much time to dedicate this week.

jayconrod · 2020-08-24T21:06:45Z

WORKSPACE

@@ -89,6 +89,9 @@ bazel_skylib_workspace()

 http_archive(
    name = "bazel_gazelle",
+    patches = [
+        "//third_party:com_google_github_bazel_gazelle-internal-in-their-new-location.patch",


I'd prefer to fix upstream issues first instead of maintaining a patch here. I'll add a note to bazel-contrib/bazel-gazelle#803, which is the only reason we have that internal dependency.

I think this is a chicken and egg situation. One of these repos will have to go first. I tried creating an alias in the BUILD file associated with it, but it turns out load statements are interpreted outside of the context of BUILD files... so I think we're stuck unfortunately. I'm happy to merge these in rapid fire though

commit rules_go with the patch

update Gazelle to refer to the new version of rules_go

update rules_go to refer to the new version of Gazelle

go/private/rules/BUILD.bazel

achew22 · 2020-08-24T22:54:53Z

@jayconrod, I'm looking through the error output:

    --- FAIL: Test/test_feature (1.74s)
        race_test.go:269: running: bazel test --features=race //:racy_test --test_arg=-wantrace=true
        race_test.go:277: unexpected build failure: exit status 1
            stderr:
            Loading: 
            Loading: 0 packages loaded
            INFO: Build option --features has changed, discarding analysis cache.
            Analyzing: target //:racy_test (0 packages loaded, 0 targets configured)
            INFO: Analyzed target //:racy_test (0 packages loaded, 7299 targets configured).
            INFO: Found 1 test target...
            [0 / 3] [Prepa] BazelWorkspaceStatusAction stable-status.txt
            ERROR: C:/b/udu4fkru/bazel_testing/bazel_go_test/main/BUILD.bazel:27:8: GoLink racy_test_/racy_test.exe failed (Exit 1)
            c:/tools/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.2.0/../../../../x86_64-w64-mingw32/lib/../lib/libmsvcrt.a(/2203): duplicate symbol reference: _unlock_file in both libgcc(.text) and libgcc(.data)
            libgcc(.text): relocation target ___chkstk_ms not defined
            libgcc(.text): relocation target atexit not defined
            link: error running the following subcommand: exit status 2
            CGO_ENABLED=1 \
            GOARCH=amd64 \
            GOOS=windows \
            GOPATH= \
            GOROOT=bazel-out/x64_windows-fastbuild/bin/external/io_bazel_rules_go/stdlib_ \
            GOROOT_FINAL=GOROOT \
            PATH=c:/tools/msys64/mingw64/bin \
            SYSTEMDRIVE=C: \
            SYSTEMROOT=C:\Windows \
            TEMP=C:\temp \
            TMP=C:\temp \
            external\go_sdk\pkg\tool\windows_amd64\link.exe -importcfg C:\b\gxuofdwa\execroot\__main__\bazel-out\x64_windows-fastbuild\bin\racy_test_\importcfg627235515 -o C:\b\gxuofdwa\execroot\__main__\bazel-out\x64_windows-fastbuild\bin\racy_test_\racy_test.exe -extld c:/tools/msys64/mingw64/bin/gcc -race -s -w -buildid=redacted C:\b\gxuofdwa\execroot\__main__\bazel-out\x64_windows-fastbuild\bin\racy_test.a
            [13 / 13] 1 / 1 tests, 1 failed; checking cached actions
            Target //:racy_test failed to build
            Use --verbose_failures to see the command lines of failed build steps.
            INFO: Elapsed time: 1.711s, Critical Path: 1.13s
            INFO: 4 processes: 4 local.
            FAILED: Build did NOT complete successfully
            FAILED: Build did NOT complete successfully

Is that the output that you would expect to see if the race were back, or has this ticked something new? Also an interesting data point, I looked up @aherrmann's run only to discover that we have the same set of failures (the complete run).

I'm happy to dig into this if it were anything other than a test with the word "race" in it. That usually means "this is something that the maintainer spent a lot of time thinking about... ask me before messing with it".

aherrmann · 2020-08-25T08:15:08Z

@achew22 Thank you for implementing this! I've confirmed that this works for my use-case.

@aherrmann, I would like to add you as a Co-authored-by on the PR before merging, is that okay with you?

Yes, absolutely. Thank you!

googlebot · 2020-08-27T05:30:09Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

achew22 · 2020-08-27T05:31:10Z

@aherrmann, I pushed the change listing you as a co-author. Could I have you appease the CLA robot?

aherrmann · 2020-08-27T09:23:48Z

@googlebot I consent.

achew22 · 2020-09-14T03:41:34Z

@jayconrod what would next steps be here? Do you want to wait more before we make this change or should I clean it up and get it passing on TAP?

jayconrod · 2020-09-14T14:03:23Z

@achew22 Updating to the latest Gazelle broke some tests. I'll do that as soon as I have the time. I tried to do it while updating the dependencies before v0.24.0, but it looked like it would take a few hours, and since it's just a test dependency, I put it on the back burner.

I apologize for the delay. This is an extremely busy time. I've been trying to get a few features into the Go command before we can flip GO111MODULE=on by default in development (golang/go#41330). We need to do that as early within the development cycle as possible.

achew22 · 2020-09-14T14:09:33Z

No worries at all. That work is important ecosystems wide and I appreciate you prioritizing it above this. I'll ping you again about this after a small delay. Thanks for all that you do!

achew22 · 2020-09-21T15:20:57Z

Friendly ping

jayconrod · 2020-09-21T15:30:10Z

I should be able to get to this in a couple days. I was sprinting to get CL 255052 and its prerequisite features merged. Now that it's done, I should have time to work on more things.

achew22 · 2020-09-21T16:30:45Z

@jayconrod I won't do this on the gerrit because it has such a wide distribution and I don't wanna bother ppl, but I wanted to say, great job! You pour so much of yourself into this and have made it a delightful place to code. Thank you, Jay!

hugelgupf · 2020-09-22T08:12:21Z

CLA bot doesn't seem to recognize the co-author email address in the commit as one of @aherrmann's, just FYI. I can't figure out why, though.

jayconrod · 2020-09-22T17:39:32Z

This should be unblocked by #2658.

CLA verified manually.

achew22 · 2020-09-22T19:39:29Z

@jayconrod sorry but pushing broke the CLA again 😦 could I have you poke it one more time?

achew22 · 2020-10-13T07:38:18Z

@jayconrod I took a look at updating this to the main branch but I have to admit I'm slightly confused.

There are a number of places where I think there is a dangling import. For example, in transition.bzl the load calls for LINKMODES from :mode.bzl. When I look in that same wd, go/private/rules, I don't see a file with that name. I do, however, see that defined in a file at go/private/mode.bzl, but that doesn't exist in the correct directory.

Maybe I'm doing something really silly, but I don't understand how this code works. My rule of thumb is, when something does work but you can't explain how it does it's a good time to ask questions. So, can you help me understand how these imports are working? Is there some kind of PATH variable for imports? Do imports occur relative to the place that they are being imported from? Am I completely wrong here in my understanding of the dir structure?

Sorry to put the burdeon back onto you, but I'm mighty confused. I'll take another look in the AM, but I wanted to write out my thoughts.

Thanks again!

jayconrod · 2020-10-13T14:20:55Z

Labels like :mode.bzl refer to files in the same Bazel package using a path relative to the directory containing the BUILD or BUILD.bazel file. So from go/private/rules/transition.bzl, the enclosing build file is go/private/BUILD.bazel, so the path is relative to go/private.

Since this PR adds build files in directories that didn't have them, some of those loads will need adjustment. That one will need to be changed to //go/private:mode.bzl.

google-cla · 2020-10-22T06:04:53Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

achew22 · 2020-10-22T06:06:03Z

Interesting discovery, looks like rules_proto was already accidentally upgraded!

I was expecting

https://github.com/bazelbuild/rules_go/blob/99e0cf0e35f21ae9ba601e3c42d133c23abf1c25/WORKSPACE#L30-L39

To be the thing that imported rules_proto, but it turns out that when com_google_protobuf was upgraded, the preexisting invocation of

https://github.com/bazelbuild/rules_go/blob/99e0cf0e35f21ae9ba601e3c42d133c23abf1c25/WORKSPACE#L22-L24

Now invokes

https://github.com/protocolbuffers/protobuf/blob/666b592e36fbebeaaccbee8714f50ed81e801fee/protobuf_deps.bzl#L51-L57

Which brings in the version of rules_proto that I thought we needed to upgrade to! So I removed it in this CL from the WORKSPACE to make things a little less confusing.

google-cla · 2020-10-22T06:32:22Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

google-cla · 2020-10-22T06:32:49Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

google-cla · 2020-10-22T06:49:28Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

google-cla · 2020-10-22T07:14:26Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

google-cla · 2020-10-22T07:24:34Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

jayconrod

Looks good I think.

CI seems to have gone off into the weeds, so I've canceled it and restarted.

jayconrod · 2020-10-22T15:23:14Z

CI's remote cache seems to be down. Opened bazelbuild/continuous-integration#1055 for tracking.

This is a large enough change that I'd like to wait for CI to be green on all platforms before submitting.

achew22 · 2020-10-22T15:59:13Z

This is a large enough change that I'd like to wait for CI to be green on all platforms before submitting.

I couldn't agree more. Thanks for not submitting it.

achew22 · 2020-10-23T05:54:28Z

Looks like CI is happier with this now. Could you take another look? @jayconrod

jayconrod · 2020-10-23T18:50:10Z

Yep, all clear. Thanks for persevering on this!

achew22 requested a review from jayconrod as a code owner August 24, 2020 18:59

googlebot added the cla: yes label Aug 24, 2020

This was referenced Aug 24, 2020

rules_go provides no bzl_library targets #2619

Closed

Add bzl_library targets for Stardoc compatibility #2620

Closed

gazelle_binary: deprecate and remove mode attributes bazel-contrib/bazel-gazelle#803

Closed

jayconrod reviewed Aug 24, 2020

View reviewed changes

achew22 mentioned this pull request Aug 24, 2020

Depend on rules_go internals in their new location bazel-contrib/bazel-gazelle#879

Closed

achew22 mentioned this pull request Aug 27, 2020

gazelle_binary: remove mode attributes bazel-contrib/bazel-gazelle#882

Merged

achew22 force-pushed the bzl branch from 88800de to cbbba90 Compare August 27, 2020 05:30

googlebot added cla: no and removed cla: yes labels Aug 27, 2020

achew22 force-pushed the bzl branch from cbbba90 to 74a17bc Compare August 27, 2020 05:32

achew22 force-pushed the bzl branch from b85607c to fe09a5f Compare August 27, 2020 21:40

achew22 force-pushed the bzl branch from fe09a5f to be6da02 Compare September 22, 2020 19:29

achew22 requested a review from jayconrod September 22, 2020 19:39

UebelAndre mentioned this pull request Oct 17, 2020

Added bzl_library targets for use in external repos using stardoc bazel-contrib/bazel-gazelle#936

Closed

achew22 mentioned this pull request Oct 20, 2020

Add bzl_library targets to Gazelle bazel-contrib/bazel-gazelle#760

Merged

achew22 added 2 commits October 22, 2020 05:49

Change all load statements

82dbdaf

Generate BUILD files using bzl plugin

e89d25e

achew22 force-pushed the bzl branch 2 times, most recently from 3ab5d55 to e72b0f5 Compare October 22, 2020 06:04

google-cla bot added cla: yes and removed cla: no labels Oct 22, 2020

achew22 force-pushed the bzl branch from e72b0f5 to 5625751 Compare October 22, 2020 06:32

Manual fixups

073384d

achew22 force-pushed the bzl branch from 5625751 to 073384d Compare October 22, 2020 06:32

achew22 force-pushed the bzl branch from c13dab2 to 8077300 Compare October 22, 2020 07:14

Recursive filegroups

d2940b9

achew22 force-pushed the bzl branch from 8077300 to d2940b9 Compare October 22, 2020 07:24

jayconrod approved these changes Oct 22, 2020

View reviewed changes

jayconrod merged commit 2bada59 into bazel-contrib:master Oct 23, 2020

achew22 deleted the bzl branch October 23, 2020 22:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create bzl_library entries using gazelle-skylark #2621

Create bzl_library entries using gazelle-skylark #2621

achew22 commented Aug 24, 2020

achew22 commented Aug 24, 2020

jayconrod left a comment

jayconrod Aug 24, 2020

achew22 Aug 24, 2020

achew22 commented Aug 24, 2020

aherrmann commented Aug 25, 2020

googlebot commented Aug 27, 2020

achew22 commented Aug 27, 2020

aherrmann commented Aug 27, 2020

achew22 commented Sep 14, 2020

jayconrod commented Sep 14, 2020

achew22 commented Sep 14, 2020

achew22 commented Sep 21, 2020

jayconrod commented Sep 21, 2020

achew22 commented Sep 21, 2020

hugelgupf commented Sep 22, 2020

jayconrod commented Sep 22, 2020

achew22 commented Sep 22, 2020

achew22 commented Oct 13, 2020

jayconrod commented Oct 13, 2020

google-cla bot commented Oct 22, 2020

achew22 commented Oct 22, 2020

google-cla bot commented Oct 22, 2020

google-cla bot commented Oct 22, 2020

google-cla bot commented Oct 22, 2020

google-cla bot commented Oct 22, 2020

google-cla bot commented Oct 22, 2020

jayconrod left a comment

jayconrod commented Oct 22, 2020

achew22 commented Oct 22, 2020

achew22 commented Oct 23, 2020

jayconrod commented Oct 23, 2020

Create bzl_library entries using gazelle-skylark #2621

Create bzl_library entries using gazelle-skylark #2621

Conversation

achew22 commented Aug 24, 2020

achew22 commented Aug 24, 2020

jayconrod left a comment

Choose a reason for hiding this comment

jayconrod Aug 24, 2020

Choose a reason for hiding this comment

achew22 Aug 24, 2020

Choose a reason for hiding this comment

achew22 commented Aug 24, 2020

aherrmann commented Aug 25, 2020

googlebot commented Aug 27, 2020

achew22 commented Aug 27, 2020

aherrmann commented Aug 27, 2020

achew22 commented Sep 14, 2020

jayconrod commented Sep 14, 2020

achew22 commented Sep 14, 2020

achew22 commented Sep 21, 2020

jayconrod commented Sep 21, 2020

achew22 commented Sep 21, 2020

hugelgupf commented Sep 22, 2020

jayconrod commented Sep 22, 2020

achew22 commented Sep 22, 2020

achew22 commented Oct 13, 2020

jayconrod commented Oct 13, 2020

google-cla bot commented Oct 22, 2020

achew22 commented Oct 22, 2020

google-cla bot commented Oct 22, 2020

google-cla bot commented Oct 22, 2020

google-cla bot commented Oct 22, 2020

google-cla bot commented Oct 22, 2020

google-cla bot commented Oct 22, 2020

jayconrod left a comment

Choose a reason for hiding this comment

jayconrod commented Oct 22, 2020

achew22 commented Oct 22, 2020

achew22 commented Oct 23, 2020

jayconrod commented Oct 23, 2020