bazelbuild / rules_go

Go rules for Bazel
Apache License 2.0
1.35k stars 635 forks source link

Upgrade Stardoc to 0.6.2 . #3966

Open lberki opened 1 week ago

lberki commented 1 week ago

This is in turn to make rules_go work with --experimental_sibling_repository_layout or at least makes bazel build --experimental_sibling_repository_layout //docs:docs_go_extras_extras-docgen pass.

It required some re-shuffling of the stanzas in the WORKSPACE file for rules_proto, Stardoc and rules_go itself. I did not investigate deeper given that the plan of record is to migrate to bzlmod. What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Makes rules_go work with --experimental_sibling_repository_layout. Or at least fixes one problem. Which issues(s) does this PR fix?

Fixes #3947

Other notes for review

lberki commented 1 week ago

Argh. The CI failure seems to be reproducible by

bazel build --nobuild --incompatible_enable_proto_toolchain_resolution @@io_bazel_stardoc//stardoc/proto:stardoc_output_java_proto

My guess: the upgrade to Stardoc also requires an upgrade of rules_java on their side so that it works with --incompatible_enable_proto_toolchain_resolution.

fmeum commented 1 week ago

Could you also add the flag here: https://github.com/bazelbuild/rules_go/blob/d3ecd76f872a96745ca2b68c1598628458dcc54c/.bazelrc#L40 and here: https://github.com/bazelbuild/rules_go/blob/d3ecd76f872a96745ca2b68c1598628458dcc54c/.bazelrc#L36

Without this, we won't have good CI coverage as most tests are integration tests.

The error about invalid escapes is related to rules_go using custom templates. I could look into getting rid of them if that simplifies the update.

lberki commented 1 week ago

Could you also add the flag here: It's there if you scroll right, on both of these lines :)

I haven't looked at the error about invalid escapes, since, well, fixing the other one seems to be complicated enough so I'll go with the generic uninformed opinion that it'd be great help; otherwise I don't know what exactly in the Stardoc update broke this; I wanted to look at it after I figure out the proto toolchain issue.

lberki commented 1 week ago

The Stardoc issue is fixed in https://github.com/bazelbuild/stardoc/pull/237 (I tested the two both). I also managed to reproduce the escaping issue without requiring Windows by the sneaky method of calling

bazel build //docs:docs_go_core_rules-docgen --incompatible_enable_proto_toolchain_resolution

fmeum commented 1 week ago

Okay, I can look into it after the stardoc fix has been released, together with the general migration required at that point.

lberki commented 1 week ago

Well, I was hoping it'd be a simple thing but turns out, not so; this is my first encounter with the magnificent Velocity Template Language and all my naive attempts to circumvent the backspace limitation failed.

fmeum commented 1 week ago

I would be in favor of getting rid of the custom templates in favor of whatever stardoc ships by default. If that gets rid of the escaping issue, that's a plus.

lberki commented 1 week ago

That is beyond my mandate here, I'm afraid; I was trying to do some adversarial programming to find a way to get a backslash into that string literal, to no avail, so the best I could do is to add some hackish method to MarkdownUtil to inject backslashes somehow.

Ain't nice, but it would work.