Fix artifact/symlink mismatch detection #15700

fmeum · 2022-06-18T19:07:08Z

The check in ActionMetadataHandler that an output declared to be a
symlink is indeed created as a symlink by the action was ineffective as
it would only run if a stat of the output showed that is already was a
symlink. The test only passed by accident since it runs sandboxed and
the sandbox setup would call Path.readSymbolicLink on what it expects to
be a symlink. As this does not happen on Windows, the test correctly
fails there.

This is fixed by calling Path.readSymbolicLink on the output path of
an expected symlink and handling errors appropriately rather
than
assuming the type returned by stat matches the artifact type (file vs symlink).

With this issue fixed, bazel_symlink_test can be enabled on Windows with
the following test-only changes:

Pass --windows_enable_symlinks as a startup option.
Use relative instead of absolute symlink targets as these have
consistent shape under Unix and Windows.
Use python to create symlinks rather than ln -s, which doesn't seem
to be able to create unresolved symlinks on Windows.

Work towards #10298

fmeum · 2022-06-29T16:34:32Z

@lberki Could you take a look? This should handle the Windows part of #10298 and fixes an actual bug along the way.

fmeum · 2022-06-30T16:21:52Z

@coeuvre Could you take over the review while @lberki is OOO?

fmeum · 2022-07-12T16:25:15Z

@coeuvre Friendly ping

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java

src/test/shell/bazel/bazel_symlink_test.sh

src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java

src/test/shell/bazel/bazel_symlink_test.sh

The check in ActionMetadataHandler that an output declared to be a symlink is indeed created as a symlink by the action was ineffective as it would only run if a stat of the output showed that is already was a symlink. The test only passed by accident since it runs sandboxed and the sandbox setup would call Path.readSymbolicLink on what it expects to be a symlink. As this does not happen on Windows, the test correctly fails there. This is fixed by calling Path.readSymbolicLink on the output path of an expected symlink before rather than after stat-ing it and trusting the file type contained in the stat info. With this issue fixed, bazel_symlink_test can be enabled on Windows with the following test-only changes: * Pass --windows_enable_symlinks as a startup option. * Use relative instead of absolute symlink targets as these have consistent shape under Unix and Windows. * Use python to create symlinks rather than `ln -s`, which doesn't seem to be able to create unresolved symlinks on Windows.

src/test/shell/bazel/bazel_symlink_test.sh

alexjski · 2022-07-29T16:40:58Z

src/test/shell/bazel/bazel_symlink_test.sh

+  # We use python rather than a simple ln since the latter doesn't handle dangling symlinks on
+  # Windows.
+  mkdir -p symlink_helper
+  cat > symlink_helper/BUILD <<EOF


nit: 'EOF' is generally preferred unless you need variable substitution. If you are OK with this, I am happy to change that as part of the import process.

Could you take care of it? I'm on mobile atm. Thanks!

alexjski · 2022-07-29T16:49:15Z

Another minor edit could be useful in the commit message:

This is fixed by calling Path.readSymbolicLink on the output path of an
expected symlink before rather than after stat-ing it and trusting the
file type contained in the stat info.

I think we established that trusting stat output is actually OK, it's just that we didn't look whether it matches the expectation, not that it cannot be used.

alexjski · 2022-07-29T16:53:13Z

src/test/shell/bazel/bazel_symlink_test.sh

@@ -370,25 +357,35 @@ def _a_impl(ctx):
    )
    return DefaultInfo(files = depset([output]))

-a = rule(implementation = _a_impl, attrs = {"link_target": attr.string()})
+a = rule(
+    implementation = _a_impl, 


FYI: I will remove the trailing space from this line as part of importing this change. You can delete that yourself if you prefer.

fmeum · 2022-07-29T17:20:22Z

Another minor edit could be useful in the commit message:
This is fixed by calling Path.readSymbolicLink on the output path of an
expected symlink before rather than after stat-ing it and trusting the
file type contained in the stat info.
I think we established that trusting stat output is actually OK, it's just that we didn't look whether it matches the expectation, not that it cannot be used.

I updated the PR description and am fine with you taking care of the nits.

Thanks for the review!

alexjski · 2022-07-29T17:23:43Z

Another minor edit could be useful in the commit message:
This is fixed by calling Path.readSymbolicLink on the output path of an
expected symlink before rather than after stat-ing it and trusting the
file type contained in the stat info.
I think we established that trusting stat output is actually OK, it's just that we didn't look whether it matches the expectation, not that it cannot be used.
I updated the PR description and am fine with you taking care of the nits.

Thanks for the review!

more nitting in the new sentence I would replace "trusting" with something like "assuming the type matches the artifact type (file vs symlink)."

fmeum · 2022-07-29T17:37:20Z

Changed. Not sure I wrapped the lines correctly though, so maybe reflow the text during the import.

alexjski · 2022-07-29T18:50:16Z

Changed. Not sure I wrapped the lines correctly though, so maybe reflow the text during the import.

Thank you! FYI, I am in the process of merging that internally now. All tests passed, so looking good. Will update the PR once it is merged. Thank you so much for creating this change (and enduring my nitpicking)! It was a pleasure to work with you on that!

alexjski · 2022-08-01T22:41:31Z

The change was merged in 666fce5. Thank you for the contribution!

fmeum force-pushed the unresolved-symlinks-windows branch 2 times, most recently from 73ff93a to 8f5d597 Compare June 19, 2022 16:00

fmeum force-pushed the unresolved-symlinks-windows branch 2 times, most recently from bf0e72e to fcef379 Compare June 29, 2022 16:11

fmeum changed the title ~~TEMP: Unresolved symlinks windows~~ Fix artifact/symlink mismatch detection Jun 29, 2022

fmeum marked this pull request as ready for review June 29, 2022 16:33

sgowroji added team-Core Skyframe, bazel query, BEP, options parsing, bazelrc awaiting-review PR is awaiting review from an assigned reviewer labels Jun 30, 2022

This was referenced Jun 30, 2022

WIP: Preserve unnormalized unresolved symlinks #15773

Closed

Allow unresolved symlinks inside a tree artifact #15454

Open

fmeum mentioned this pull request Jul 20, 2022

[WIP] Unresolved symlinks #15866

Closed

haxorz requested a review from alexjski July 25, 2022 14:23

alexjski suggested changes Jul 27, 2022

View reviewed changes

fmeum force-pushed the unresolved-symlinks-windows branch from fcef379 to 31618be Compare July 28, 2022 09:57

alexjski reviewed Jul 28, 2022

View reviewed changes

src/test/shell/bazel/bazel_symlink_test.sh Outdated Show resolved Hide resolved

fmeum force-pushed the unresolved-symlinks-windows branch from 13caa68 to 9700359 Compare July 28, 2022 13:46

fmeum force-pushed the unresolved-symlinks-windows branch from 9700359 to 0e43c7c Compare July 28, 2022 13:47

alexjski approved these changes Jul 28, 2022

View reviewed changes

alexjski suggested changes Jul 28, 2022

View reviewed changes

src/test/shell/bazel/bazel_symlink_test.sh Outdated Show resolved Hide resolved

fmeum force-pushed the unresolved-symlinks-windows branch 2 times, most recently from d6e4aaf to bf04a68 Compare July 28, 2022 22:02

Simplify symlink helper

8cbf26a

fmeum force-pushed the unresolved-symlinks-windows branch from bf04a68 to 8cbf26a Compare July 28, 2022 22:07

fmeum commented Jul 28, 2022

View reviewed changes

src/test/shell/bazel/bazel_symlink_test.sh Show resolved Hide resolved

alexjski reviewed Jul 29, 2022

View reviewed changes

copybara-service bot closed this in 666fce5 Aug 1, 2022

ShreeM01 removed the awaiting-review PR is awaiting review from an assigned reviewer label Sep 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix artifact/symlink mismatch detection #15700

Fix artifact/symlink mismatch detection #15700

fmeum commented Jun 18, 2022 •

edited

Loading

fmeum commented Jun 29, 2022

fmeum commented Jun 30, 2022

fmeum commented Jul 12, 2022

alexjski Jul 29, 2022 •

edited

Loading

fmeum Jul 29, 2022

alexjski commented Jul 29, 2022

alexjski Jul 29, 2022

fmeum Jul 29, 2022

fmeum commented Jul 29, 2022

alexjski commented Jul 29, 2022

fmeum commented Jul 29, 2022

alexjski commented Jul 29, 2022

alexjski commented Aug 1, 2022

Fix artifact/symlink mismatch detection #15700

Fix artifact/symlink mismatch detection #15700

Conversation

fmeum commented Jun 18, 2022 • edited Loading

fmeum commented Jun 29, 2022

fmeum commented Jun 30, 2022

fmeum commented Jul 12, 2022

alexjski Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

fmeum Jul 29, 2022

Choose a reason for hiding this comment

alexjski commented Jul 29, 2022

alexjski Jul 29, 2022

Choose a reason for hiding this comment

fmeum Jul 29, 2022

Choose a reason for hiding this comment

fmeum commented Jul 29, 2022

alexjski commented Jul 29, 2022

fmeum commented Jul 29, 2022

alexjski commented Jul 29, 2022

alexjski commented Aug 1, 2022

fmeum commented Jun 18, 2022 •

edited

Loading

alexjski Jul 29, 2022 •

edited

Loading