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

Upgrade Stardoc to 0.6.2 . #3966

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

lberki
Copy link
Contributor

@lberki lberki commented Jun 20, 2024

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

This is in turn to make rules_go work with
`--experimental_sibling_repository_layout`.

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.
@lberki
Copy link
Contributor Author

lberki commented Jun 20, 2024

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
Copy link
Collaborator

fmeum commented Jun 20, 2024

Could you also add the flag here:

test:incompatible --test_env=GO_BAZEL_TEST_BAZELFLAGS='--incompatible_load_proto_rules_from_bzl --incompatible_enable_cc_toolchain_resolution --incompatible_config_setting_private_default_visibility --incompatible_enforce_config_setting_visibility --incompatible_disable_starlark_host_transitions --nolegacy_external_runfiles --incompatible_enable_proto_toolchain_resolution'

and here:
build:incompatible --incompatible_enable_proto_toolchain_resolution

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
Copy link
Contributor Author

lberki commented Jun 20, 2024

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
Copy link
Contributor Author

lberki commented Jun 21, 2024

The Stardoc issue is fixed in bazelbuild/stardoc#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
Copy link
Collaborator

fmeum commented Jun 21, 2024

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

@lberki
Copy link
Contributor Author

lberki commented Jun 21, 2024

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
Copy link
Collaborator

fmeum commented Jun 21, 2024

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
Copy link
Contributor Author

lberki commented Jun 21, 2024

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.

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.

rules_go + protobufs + experimental_sibling_repository_layout fails to build
2 participants