Make Stardoc repository mapping aware #16775

fmeum · 2022-11-16T02:17:34Z

By taking in .bzl files as runfiles rather than inputs, Stardoc can use the existing Java runfiles library to resolve apparent repository names in load labels into canonical repository names.

Work towards #16124
Fixes #14140

fmeum · 2022-11-16T03:00:25Z

@comius Combined with bazelbuild/stardoc#141, which this PR is referencing, I got Stardoc to work with Bzlmod. Both PRs need some cleanup, but early feedback on the general approach would be appreciated.

fmeum · 2022-11-18T20:52:37Z

@comius This should be ready for review now.

comius · 2022-12-05T10:53:56Z

distdir_deps.bzl

@@ -274,16 +274,15 @@ DIST_DEPS = {
        ],
    },
    "io_bazel_skydoc": {
-        "archive": "1ef781ced3b1443dca3ed05dec1989eca1a4e1cd.tar.gz",
-        "sha256": "5a725b777976b77aa122b707d1b6f0f39b6020f66cd427bb111a585599c857b1",
+        "archive": "d09735f49f046fdebf2fc36312dc9f021f89ce27.tar.gz",


This points out of main branch of stardoc and as such it should probably be reverted.

What is the right order? I'm guessing first merge this PR, then release the binary in stardoc repo with .bzl fixes.

Yes, I only added this here to make more of the tests pass. I will revert this part and check whether that can be merged.

src/main/java/com/google/devtools/build/skydoc/SkydocMain.java

fmeum · 2022-12-05T15:41:47Z

@comius Tests are failing here if I don't also update the pin to the stardoc rules. Since the same is true for bazelbuild/stardoc#141, I don't see a way to merge this that doesn't involve temporarily pointing one repo at a non-main branch of the other.

fmeum · 2022-12-05T15:54:54Z

@comius How about the following:

After reviewing Add support for Bzlmod stardoc#141 and before merging it to master, you create a trusted branch from it.
I update the pin in the current PR to point to that branch's HEAD commit, temporarily removing the mirror URL.
You merge this Bazel PR.
I update Add support for Bzlmod stardoc#141 to reference that Bazel commit, getting the tests to pass.
You merge Add support for Bzlmod stardoc#141.
I submit a new Bazel PR together with a mirror request for stardoc, changing the distdir dep for stardoc back to stardoc master.

comius · 2022-12-09T10:18:44Z

@comius How about the following:

After reviewing Add support for Bzlmod stardoc#141 and before merging it to master, you create a trusted branch from it.

I update the pin in the current PR to point to that branch's HEAD commit, temporarily removing the mirror URL.

You merge this Bazel PR.

I update Add support for Bzlmod stardoc#141 to reference that Bazel commit, getting the tests to pass.

You merge Add support for Bzlmod stardoc#141.

I submit a new Bazel PR together with a mirror request for stardoc, changing the distdir dep for stardoc back to stardoc master.

SQTM

brentleyjones · 2023-01-19T17:34:38Z

Friendly ping on this 🙏.

tools/java/runfiles/Runfiles.java

tools/java/runfiles/BUILD

src/main/java/com/google/devtools/build/skydoc/SkydocMain.java

comius · 2023-03-08T14:12:34Z

tools/java/runfiles/RunfilesForStardoc.java

+
+package com.google.devtools.build.runfiles;
+
+public class RunfilesForStardoc {


Lint suggests: Classes which are not intended to be instantiated should be made non-instantiable with a private constructor. This includes utility classes (classes with only static members), and the main class.

Make the class final and add a private no-op constructor.

I can fix that, but should I repair the merge first?

feel free to drop the merge commit and do a rebase (or anything else you might prefer)

comius · 2023-03-08T14:42:02Z

tools/java/runfiles/RunfilesForStardoc.java

+package com.google.devtools.build.runfiles;
+
+public class RunfilesForStardoc {
+  public static String getCanonicalRepository(Runfiles runfiles, String sourceRepository) {


linter wants a javadoc on public methods :)

and on the class

Added. I also renamed the parameter.

comius · 2023-03-08T14:44:08Z

src/test/java/com/google/devtools/build/skydoc/BUILD

@@ -23,18 +23,16 @@ java_test(
    deps = [
        "//src/main/java/com/google/devtools/build/lib/cmdline",
        "//src/main/java/com/google/devtools/build/lib/packages/semantics",
-        "//src/main/java/com/google/devtools/build/lib/vfs",
-        "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
        "//src/main/java/com/google/devtools/build/skydoc:skydoc_lib",
        "//src/main/java/com/google/devtools/build/skydoc/fakebuildapi",
        "//src/main/java/com/google/devtools/build/skydoc/rendering",
        "//src/main/java/com/google/devtools/build/skydoc/rendering/proto:stardoc_output_java_proto",
        "//src/main/java/net/starlark/java/eval",
        "//src/main/java/net/starlark/java/syntax",


syntax also seems unused

It is used for Resolver.

By taking in .bzl files as runfiles rather than inputs, Stardoc can use the existing Java runfiles library to resolve apparent repository names in load labels into canonical repository names.

fmeum · 2023-03-16T09:36:08Z

@comius Friendly ping, is there anything else I should do to move this forward?

comius · 2023-03-16T09:55:12Z

@comius Friendly ping, is there anything else I should do to move this forward?

In the last 2 weeks I did a reimport of stardoc, which didn't happen for 2 years and was tricky.

Generally the changes are ok, but since now stardoc needs runfiles and when they are handled using absolute paths we'll need to account for this. I checked and this can't be done by external contributor. Good thing is, that accounting for differences most likely won't be visible in PRs.

Also the plan looks ok. One option is that you go forward with the plan, and I account for the differences on each step. Second option is that I import everything, figure out how to account for the differences and then you go forward. This derisks it a bit.

I'm out of office until 27th March, but I'll pick it up when I'm back

Third option is that @Wyverald takes it over if there's more hurry than this.

fmeum · 2023-03-16T10:04:08Z

@comius Thank you very much for the transparency! There is no hurry, it's just difficult for a non-Googler like me to distinguish between something having been forgotten/deprioritized or something being stuck on some complicated google3 import. Thanks again for describing the options.

meteorcloudy · 2023-04-11T12:18:00Z

Gently ping @comius

comius · 2023-04-14T15:43:18Z

Hey, the migration plan doesn't work, because we're building from sources. Because of that the changes to stardoc.bzl and StardocMain.java need to happen atomically.

I think I have an alternative:

first we copy stardoc.bzl into the bazel repostory and remove the stardoc dependency
then I import both PRs at the same time, but export only to Bazel; Bazel CI will pass
then we can merge Stardoc PR directly into main, which will reconcile the split worlds

fmeum · 2023-04-14T15:50:26Z

@comius Sounds good to me, although I don't understand all the implications well enough to assess whether this will work or not.

Do you want me to perform/prepare any of these steps or is it easier if you do it directly via CLs?

comius · 2023-04-17T10:26:58Z

@comius Sounds good to me, although I don't understand all the implications well enough to assess whether this will work or not.

Do you want me to perform/prepare any of these steps or is it easier if you do it directly via CLs?

I've imported both PRs today, they are now in the internal review.

Before Stardoc implementation was tested using stardoc.bzl from Stardoc repository. This makes it hard to change the implementation and bzl file at the same time. The change is preparation to import #16775 and bazelbuild/stardoc#141. Without it we'd need to do a half-release of Stardoc PR to make Bazel's CI pass. There's still a dependency on Stardoc repo/module from Bazel, which is used to generate the docs. PiperOrigin-RevId: 525736214 Change-Id: I291838a9dd9bbb9482a47011e6e36fc6dd67eff1

Before Stardoc implementation was tested using stardoc.bzl from Stardoc repository. This makes it hard to change the implementation and bzl file at the same time. The change is preparation to import bazelbuild#16775 and bazelbuild/stardoc#141. Without it we'd need to do a half-release of Stardoc PR to make Bazel's CI pass. There's still a dependency on Stardoc repo/module from Bazel, which is used to generate the docs. PiperOrigin-RevId: 525736214 Change-Id: I291838a9dd9bbb9482a47011e6e36fc6dd67eff1

By taking in .bzl files as runfiles rather than inputs, Stardoc can use the existing Java runfiles library to resolve apparent repository names in load labels into canonical repository names. Work towards bazelbuild#16124 Fixes bazelbuild#14140 Closes bazelbuild#16775. PiperOrigin-RevId: 526976752 Change-Id: I0848a5c7590348f778ad8c939fd37c89a53e55b2

fmeum force-pushed the 16124-stardoc branch 2 times, most recently from 7807e42 to 96d65bc Compare November 16, 2022 02:47

fmeum force-pushed the 16124-stardoc branch from 96d65bc to f97827e Compare November 16, 2022 03:35

fmeum marked this pull request as ready for review November 18, 2022 20:51

ShreeM01 added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels Nov 18, 2022

meteorcloudy requested a review from comius December 2, 2022 16:12

comius requested changes Dec 5, 2022

View reviewed changes

fmeum force-pushed the 16124-stardoc branch 2 times, most recently from 79648ee to 5100f17 Compare December 5, 2022 15:30

fmeum requested a review from comius December 5, 2022 15:42

fmeum mentioned this pull request Jan 5, 2023

Add support for Bzlmod bazelbuild/stardoc#141

Merged

fmeum mentioned this pull request Jan 12, 2023

Using bzlmod makes stardoc unable to find dependencies bazelbuild/stardoc#117

Closed

Wyverald requested a review from tetromino January 25, 2023 14:29

comius reviewed Mar 8, 2023

View reviewed changes

tools/java/runfiles/Runfiles.java Outdated Show resolved Hide resolved

comius reviewed Mar 8, 2023

View reviewed changes

tools/java/runfiles/BUILD Show resolved Hide resolved

comius reviewed Mar 8, 2023

View reviewed changes

src/main/java/com/google/devtools/build/skydoc/SkydocMain.java Outdated Show resolved Hide resolved

comius reviewed Mar 8, 2023

View reviewed changes

Make Stardoc repository mapping aware

6a4b9fa

By taking in .bzl files as runfiles rather than inputs, Stardoc can use the existing Java runfiles library to resolve apparent repository names in load labels into canonical repository names.

fmeum force-pushed the 16124-stardoc branch from 2038f4d to 6a4b9fa Compare March 8, 2023 16:02

fmeum requested review from comius and removed request for tetromino March 8, 2023 16:03

copybara-service bot closed this in fde2479 Apr 25, 2023

fmeum deleted the 16124-stardoc branch April 28, 2023 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Stardoc repository mapping aware #16775

Make Stardoc repository mapping aware #16775

fmeum commented Nov 16, 2022 •

edited

fmeum commented Nov 16, 2022 •

edited

fmeum commented Nov 18, 2022 •

edited

comius Dec 5, 2022

fmeum Dec 5, 2022

fmeum commented Dec 5, 2022 •

edited

fmeum commented Dec 5, 2022

comius commented Dec 9, 2022

brentleyjones commented Jan 19, 2023

comius Mar 8, 2023

fmeum Mar 8, 2023

comius Mar 8, 2023

comius Mar 8, 2023

comius Mar 8, 2023

fmeum Mar 8, 2023 •

edited

comius Mar 8, 2023

fmeum Mar 8, 2023

fmeum commented Mar 16, 2023

comius commented Mar 16, 2023

fmeum commented Mar 16, 2023

meteorcloudy commented Apr 11, 2023

comius commented Apr 14, 2023

fmeum commented Apr 14, 2023

comius commented Apr 17, 2023


		package com.google.devtools.build.runfiles;

		public class RunfilesForStardoc {

Make Stardoc repository mapping aware #16775

Make Stardoc repository mapping aware #16775

Conversation

fmeum commented Nov 16, 2022 • edited

fmeum commented Nov 16, 2022 • edited

fmeum commented Nov 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmeum commented Dec 5, 2022 • edited

fmeum commented Dec 5, 2022

comius commented Dec 9, 2022

brentleyjones commented Jan 19, 2023

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

fmeum Mar 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmeum commented Mar 16, 2023

comius commented Mar 16, 2023

fmeum commented Mar 16, 2023

meteorcloudy commented Apr 11, 2023

comius commented Apr 14, 2023

fmeum commented Apr 14, 2023

comius commented Apr 17, 2023

fmeum commented Nov 16, 2022 •

edited

fmeum commented Nov 16, 2022 •

edited

fmeum commented Nov 18, 2022 •

edited

fmeum commented Dec 5, 2022 •

edited

fmeum Mar 8, 2023 •

edited