Properly account for symlinks and root symlinks in repo mapping manifest #18381

fmeum · 2023-05-12T08:43:47Z

A runfiles symlink entry path/to/pkg/file -> path/to/artifact is implicitly prefixed with the workspace name and should thus result in repo mappings to the main repository being added to the repo mapping manifest, not mappings for the owner of the artifact.

A runfiles root symlink entry first_segment/path/to/pkg/file -> path/to/artifact should result result in repo mappings to the repository with canonical name equal to the first segment being added to the repo mapping manifest, if such a repository exists.

Along the way clarifies in the documentation of ctx.runfiles(symlinks = ...) that the workspace name will be prefixed implicitly.

src/test/java/com/google/devtools/build/lib/analysis/RunfilesRepoMappingManifestTest.java

src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleContextApi.java

src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleContextApi.java

src/test/java/com/google/devtools/build/lib/analysis/RunfilesRepoMappingManifestTest.java

rickeylev · 2023-05-14T23:22:18Z

src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java

  }
-  /** Describes the inputs {@link fingerprint} uses to aid describeKey() descriptions. */
+
+  /** Describes the inputs {@link #fingerprint} uses to aid describeKey() descriptions. */
  public String describeFingerprint() {


Can you update the describeFingerprint output to note that the reported results may be slightly misleading because addNestedSetToFingerprint will fingerprint the exact underlying structure of the nested sets, not simply the logical elements in the encountered order? It's easy to look at the output and see that the values are "equal", but you can't see the underlying nested set structure that the fingerprinting uses.

This tripped me up for a day trying to diagnose an action conflict. All the inputs were logically equivalent, but some difference in the NestedSet structures caused the fingerprinting to be different.

Was this comment meant to apply to my other PR?

I meant in this PR, though I'd give the same comment in your other PR (18419, right?).

In this PR, the fingerprint() function (which is called by computeKey somewhere, right?) is being changed to call addNestedSetToFingerprint(), which will include the exact internal structure of the nested set in the fingerprint.

Meanwhile, describeFingerprint will just render the flattened, linearized, unique, elements of the nested set. The net effect is, if an action conflict occurs, you get an error like:

Action conflict: computeKey fingerprint: 8Ef7283Ag, F83E241 files: a, b, c symlinks: d, e, f rootSymlinks: g, h, i

i.e. the contents of the two are shown to be identical. Which is confusing, because if they're the same, why is there a problem? But it can be hard to identify that they are the same if the output is large.

There's another case of these elsewhere, I think something to do with tree artifacts; describeFingerprint can't accurately/correctly tell what is in them. Which is fine, we just added a line to the output to mention the reported info may not be entirely accurate. Then you have a hint that when the fingerprint differs, but everything else looks the same, it might be something to do with describeFingerprint not being able to be 100% accurate, so you can dig deeper into that object to try and identify the difference another way.

rickeylev · 2023-05-17T19:32:10Z

src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java

  }
-  /** Describes the inputs {@link fingerprint} uses to aid describeKey() descriptions. */
+
+  /** Describes the inputs {@link #fingerprint} uses to aid describeKey() descriptions. */
  public String describeFingerprint() {


I meant in this PR, though I'd give the same comment in your other PR (18419, right?).

In this PR, the fingerprint() function (which is called by computeKey somewhere, right?) is being changed to call addNestedSetToFingerprint(), which will include the exact internal structure of the nested set in the fingerprint.

Meanwhile, describeFingerprint will just render the flattened, linearized, unique, elements of the nested set. The net effect is, if an action conflict occurs, you get an error like:

Action conflict: computeKey fingerprint: 8Ef7283Ag, F83E241 files: a, b, c symlinks: d, e, f rootSymlinks: g, h, i

i.e. the contents of the two are shown to be identical. Which is confusing, because if they're the same, why is there a problem? But it can be hard to identify that they are the same if the output is large.

There's another case of these elsewhere, I think something to do with tree artifacts; describeFingerprint can't accurately/correctly tell what is in them. Which is fine, we just added a line to the output to mention the reported info may not be entirely accurate. Then you have a hint that when the fingerprint differs, but everything else looks the same, it might be something to do with describeFingerprint not being able to be 100% accurate, so you can dig deeper into that object to try and identify the difference another way.

rickeylev · 2023-05-17T19:35:33Z

src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java

  }
-  /** Describes the inputs {@link fingerprint} uses to aid describeKey() descriptions. */
+
+  /** Describes the inputs {@link #fingerprint} uses to aid describeKey() descriptions. */
  public String describeFingerprint() {
    StringBuilder sb = new StringBuilder();
    sb.append(String.format("legacyExternalRunfiles: %s\n", legacyExternalRunfiles));


e.g. something like this, but move it above after the "suffix" line, before when the symlinks and other nested sets are flattened and reported. (github won't let me suggest the edit on the line I want to suggest it on)

Suggested change

sb.append(String.format("legacyExternalRunfiles: %s\n", legacyExternalRunfiles));

sb.append(String.format("legacyExternalRunfiles: %s\n", legacyExternalRunfiles));

sb.append("NOTE: fingerprinting considers internal nested set structure, which is not reflected in values reported below");

I think I will build this into NestedSetFingerprintCache#describeFingerprint and use that method here once #18384 has been merged. What do you think about that? It ensures that we can never miss adding that note.

Wyverald

thanks! not having much experience with the usage of ctx.runfiles myself, I don't think I can provide much valuable feedback, but I can at least say I think I understand what this PR is doing :)

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleContextApi.java

A runfiles symlink entry `path/to/pkg/file -> path/to/artifact` is implicitly prefixed with the workspace name and should thus result in repo mappings to the main repository to be added to the repo mapping manifest, not mappings for the owner of the artifact. A runfiles root symlink entry `first_segment/path/to/pkg/file -> path/to/artifact` should result result in repo mappings to the repository with canonical name equal to the first segment to be added to the repo mapping manifest, if it exists. Along the way clarifies in the documentation of `ctx.runfiles(symlinks = ...)` that the workspace name will be prefixed implicitly.

fmeum · 2023-06-20T16:56:23Z

@bazel-io flag

iancha1992 · 2023-06-20T18:26:24Z

@bazel-io fork 6.3.0

iancha1992 · 2023-06-21T17:49:36Z

@fmeum Because there were several conflict merges, there may be one or more commits needed to push before we can cherry-pick this one for 6.3.0. One of the conflicts was the missing "PyBuiltins.java" file in the current "release-6.3.0" branch.

fmeum · 2023-06-22T20:47:16Z

@iancha1992 I think that we can't reasonably cherry-pick this change. Sorry for the late insight.

iancha1992 · 2023-06-23T19:00:05Z

@iancha1992 I think that we can't reasonably cherry-pick this change. Sorry for the late insight.

cc: @Wyverald @meteorcloudy

fmeum marked this pull request as ready for review May 12, 2023 08:44

fmeum requested a review from a team as a code owner May 12, 2023 08:44

fmeum requested review from katre and removed request for a team May 12, 2023 08:44

github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions labels May 12, 2023

fmeum requested review from Wyverald and rickeylev and removed request for katre May 12, 2023 08:44

fmeum mentioned this pull request May 12, 2023

Don't eagerly flatten a NestedSet in RepoMappingManifestAction #18349

Closed

fmeum force-pushed the runfiles-symlinks-manifest branch from 925193e to fa6ff81 Compare May 12, 2023 11:25

rickeylev reviewed May 12, 2023

View reviewed changes

src/test/java/com/google/devtools/build/lib/analysis/RunfilesRepoMappingManifestTest.java Show resolved Hide resolved

rickeylev reviewed May 12, 2023

View reviewed changes

src/main/java/com/google/devtools/build/lib/analysis/RepoMappingManifestAction.java Outdated Show resolved Hide resolved

rickeylev reviewed May 12, 2023

View reviewed changes

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleContextApi.java Outdated Show resolved Hide resolved

rickeylev approved these changes May 14, 2023

View reviewed changes

fmeum requested a review from rickeylev May 16, 2023 10:06

sgowroji assigned rickeylev May 17, 2023

rickeylev requested changes May 17, 2023

View reviewed changes

fmeum force-pushed the runfiles-symlinks-manifest branch from af5d01b to 22e7a42 Compare May 24, 2023 10:00

fmeum requested a review from rickeylev May 24, 2023 10:00

fmeum force-pushed the runfiles-symlinks-manifest branch from 22e7a42 to c362d2b Compare May 24, 2023 10:47

fmeum requested a review from lberki as a code owner May 24, 2023 10:47

Wyverald approved these changes Jun 14, 2023

View reviewed changes

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleContextApi.java Show resolved Hide resolved

rickeylev approved these changes Jun 14, 2023

View reviewed changes

fmeum added 5 commits June 15, 2023 16:32

Improve comment

5fbc5bd

Address review comment

90863ef

Address review comment

13f680c

Update rules docs

0c802b3

fmeum force-pushed the runfiles-symlinks-manifest branch from c362d2b to 0c802b3 Compare June 15, 2023 14:35

fmeum requested review from gregestren, philomathing and fweikert as code owners June 15, 2023 14:35

Wyverald approved these changes Jun 15, 2023

View reviewed changes

Wyverald added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jun 15, 2023

copybara-service bot closed this in 73ed46c Jun 20, 2023

github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jun 20, 2023

fmeum deleted the runfiles-symlinks-manifest branch June 20, 2023 16:56

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jun 20, 2023

bazel-io mentioned this pull request Jun 20, 2023

[6.3.0] Properly account for symlinks and root symlinks in repo mapping manifest #18728

Closed

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jun 20, 2023

keertk mentioned this pull request Jun 28, 2023

[6.3.0] Don't eagerly flatten a NestedSet in RepoMappingManifestAction #18419

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly account for symlinks and root symlinks in repo mapping manifest #18381

Properly account for symlinks and root symlinks in repo mapping manifest #18381

fmeum commented May 12, 2023 •

edited

Loading

rickeylev May 14, 2023

fmeum May 17, 2023

rickeylev May 17, 2023

rickeylev May 17, 2023

rickeylev May 17, 2023

fmeum May 19, 2023

fmeum May 24, 2023

Wyverald left a comment

fmeum commented Jun 20, 2023

iancha1992 commented Jun 20, 2023

iancha1992 commented Jun 21, 2023

fmeum commented Jun 22, 2023

iancha1992 commented Jun 23, 2023

	sb.append(String.format("legacyExternalRunfiles: %s\n", legacyExternalRunfiles));
	sb.append(String.format("legacyExternalRunfiles: %s\n", legacyExternalRunfiles));
	sb.append("NOTE: fingerprinting considers internal nested set structure, which is not reflected in values reported below");

Properly account for symlinks and root symlinks in repo mapping manifest #18381

Properly account for symlinks and root symlinks in repo mapping manifest #18381

Conversation

fmeum commented May 12, 2023 • edited Loading

rickeylev May 14, 2023

Choose a reason for hiding this comment

fmeum May 17, 2023

Choose a reason for hiding this comment

rickeylev May 17, 2023

Choose a reason for hiding this comment

rickeylev May 17, 2023

Choose a reason for hiding this comment

rickeylev May 17, 2023

Choose a reason for hiding this comment

fmeum May 19, 2023

Choose a reason for hiding this comment

fmeum May 24, 2023

Choose a reason for hiding this comment

Wyverald left a comment

Choose a reason for hiding this comment

fmeum commented Jun 20, 2023

iancha1992 commented Jun 20, 2023

iancha1992 commented Jun 21, 2023

fmeum commented Jun 22, 2023

iancha1992 commented Jun 23, 2023

fmeum commented May 12, 2023 •

edited

Loading