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.

Sign up for GitHub

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

Jump to bottom

Include the full absolute paths of runfiles in action keys #19171

Closed
wants to merge 2 commits into from
Closed

Include the full absolute paths of runfiles in action keys #19171

wants to merge 2 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Aug 4, 2023 •
edited
Loading

Both SourceManifestAction and SymlinkTreeAction, the only users of Runfiles#fingerprint, emit absolute paths to runfiles artifacts in their output. This requires also including these paths in the action key computation to prevent incorrect incremental builds if --output_base changes.

Fixes #17267

@fmeum fmeum force-pushed the 17267-absolute-runfiles-paths branch 2 times, most recently from e07fcfb to bb35179 Compare August 4, 2023 13:32
@fmeum fmeum marked this pull request as ready for review August 4, 2023 13:32
@fmeum fmeum requested a review from a team as a code owner August 4, 2023 13:32
@fmeum fmeum requested review from aiuto and lberki and removed request for a team and aiuto August 4, 2023 13:32
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions labels Aug 4, 2023
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 4, 2023

@lberki I didn't manage to turn the reproducer in #17267 into an integration test (it always passes even without these changes), but confirmed manually that it fixes the issue.

@fmeum fmeum force-pushed the 17267-absolute-runfiles-paths branch from bb35179 to 30f3cf5 Compare August 4, 2023 14:08
lberki
lberki reviewed Aug 7, 2023
View reviewed changes
Copy link
Contributor

@lberki lberki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, looks good, but @c-parsons might have an opinion about this, let me pull him in.

src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java Outdated Show resolved Hide resolved
@lberki
Copy link
Contributor

lberki commented Aug 7, 2023

This looks good in principle, with one caveat: @c-parsons ran into a situation where it would be advantageous to be able to move Bazel output bases around in the file system while keeping as much cached as possible, and this makes that worse.

However, I think it's for the better, because it just makes the reality that the absolute path of the source tree is an input of what the source manifes action does more obvious, so let's go ahead; I'll wait for a confirmation from @c-parsons , but I'd be surprised if he had an objection.

@lberki
Copy link
Contributor

lberki commented Aug 7, 2023

Out of curiosity, how come you haven't been able to write an integration test? I thought it would be pretty easy to wrap the reproduction at https://github.com/JesseTatasciore/bazel-manifest-caching-repro into a test case.

@fmeum fmeum force-pushed the 17267-absolute-runfiles-paths branch 2 times, most recently from 5a1c4b8 to 99f863f Compare August 8, 2023 07:36
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 8, 2023

Out of curiosity, how come you haven't been able to write an integration test? I thought it would be pretty easy to wrap the reproduction at https://github.com/JesseTatasciore/bazel-manifest-caching-repro into a test case.

Tried this again and now the test is failing without and passing with the change. Not sure what I got wrong on the first attempt.

@fmeum fmeum requested a review from lberki August 8, 2023 07:36
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 8, 2023

@lberki Should I just skip the test on Windows? I don't know whether it's possible to get the unmangled PWD in Bash on Windows.

@lberki
Copy link
Contributor

lberki commented Aug 8, 2023

@meteorcloudy maybe you have an idea? I'm not very enthusiastic about skipping the test on Windows due to the difficulty of debugging Windows-only issues.

lberki
lberki reviewed Aug 8, 2023
View reviewed changes
Copy link
Contributor

@lberki lberki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks alright, modulo one nit and that I haven't heard from @c-parsons ; if he doesn't weigh in until tomorrow, let's assume he's fine with it.

src/test/shell/integration/runfiles_test.sh

mkdir -p "${OUTPUT_BASE}"
mkdir -p "${TEST_FOLDER_1}"
mkdir -p "${TEST_FOLDER_2}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: symlinking $TEST_FOLDER_2 to $TEST_FOLDER_1 makes it more obvious that the two are the same

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am copying the contents now. I think that symlinking wouldn't work as the "same workspace, same output root" case didn't result in any issues even before this change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that Bazel doesn't resolve symlinks in the name of the workspace, but you're right -- copying is the obviously correct option.

@meteorcloudy
Copy link
Member

We are using msys2 on windows, you can use cygpath to convert the path to a format that is understandable on Windows. For example, cygpath -m /c/foo/bar converts it to C:/foo/bar.

@meteorcloudy
Copy link
Member 

cat: bazel-bin/hello_world.runfiles_manifest: No such file or directory

It's bazel-bin/hello_world.exe.runfiles_manifest on Windows

@fmeum fmeum force-pushed the 17267-absolute-runfiles-paths branch from d1a19bf to ee935e4 Compare August 8, 2023 13:14
@fmeum fmeum requested a review from lberki August 8, 2023 13:37
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 8, 2023

Thanks @meteorcloudy, the tests are passing now.

@meteorcloudy
Copy link
Member

/cc @lberki LGTM?

lberki
lberki approved these changes Aug 11, 2023
View reviewed changes
Copy link
Contributor

@lberki lberki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked @c-parsons to weigh in and given that he didn't and that to the best of my knowledge, his use case is not hampered by this change, let's merge it.

@lberki lberki removed the awaiting-review PR is awaiting review from an assigned reviewer label Aug 11, 2023
@lberki lberki added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 11, 2023
@fmeum
Include the full absolute paths of runfiles in action keys
ec30828 
Both `SourceManifestAction` and `SymlinkTreeAction`, the only users of
`Runfiles#fingerprint`, emit absolute paths to runfiles artifacts in
their output. This requires also including these paths in the action key
computation to prevent incorrect incremental builds if `--output_base`
changes.
@fmeum
Address review comments
6114338
@fmeum fmeum force-pushed the 17267-absolute-runfiles-paths branch from ee935e4 to 6114338 Compare August 17, 2023 07:47
@fmeum fmeum requested a review from lberki August 17, 2023 07:47
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 17, 2023

@lberki I resolved the merge conflict.

@lberki
Copy link
Contributor

lberki commented Aug 17, 2023

No worries, we are doing the same at Google (it just takes a bit of time, sorry about that)

@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 18, 2023
@fmeum fmeum deleted the 17267-absolute-runfiles-paths branch August 21, 2023 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lberki lberki Awaiting requested review from lberki

Assignees
No one assigned
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

runfiles_manifest cached incorrectly with output_user_root and output_base flags set
3 participants
@fmeum @lberki @meteorcloudy