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

Prevent non-hermetic worker flagfile reads #18156

Closed
wants to merge 1 commit into from
Closed

Prevent non-hermetic worker flagfile reads #18156

wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Apr 20, 2023 •
edited
Loading

A worker command line referencing a flag file that is not an input to the action now results in an error instead of a non-hermetic file read.

Work towards #6526

@fmeum
Prevent non-hermetic worker flagfile reads
d288854 
A worker command line referencing a flag file that is not an input to
the action now results in an error instead of a non-hermetic file read.
@fmeum
Copy link
Collaborator Author

fmeum commented Apr 20, 2023

@larsrc-google Could you review this PR? I noticed this issue while working on #18155, but think that improving this could be useful independent of path mapping.

@fmeum fmeum marked this pull request as ready for review April 20, 2023 14:35
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Local-Exec Issues and PRs for the Execution (Local) team labels Apr 20, 2023
@keertk keertk requested a review from larsrc-google April 20, 2023 14:46
@fmeum fmeum mentioned this pull request Apr 20, 2023
Closed
@larsrc-google
Copy link
Contributor

This seems like a good idea, but I wonder how much it's going to break. Testing it internally now. It could go well with --experimental_worker_strict_flagfiles, though that really ought to be named --incompatible_worker_strict_flagfiles

@fmeum
Copy link
Collaborator Author

fmeum commented Apr 21, 2023 •
edited
Loading

We could introduce an alternative name for that flag. Since param files should be added to the inputs automatically by Bazel's command line invocations, I hope that this won't end up breaking anything, but I agree it's hard to assess that.

Is it planned to flip this flag in the near future (e.g. for Bazel 7)?

@fmeum
Copy link
Collaborator Author

fmeum commented May 11, 2023

@larsrc-google Were you able to test this internally yet?

@larsrc-google
Copy link
Contributor

Have now, and I didn't find any problems.

GTech: Since I've already imported it, I'll ask for review directly

@copybara-service copybara-service bot closed this in 36b2f8c Jun 5, 2023
@fmeum fmeum deleted the non-hermetic-flagfile branch June 6, 2023 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larsrc-google larsrc-google larsrc-google approved these changes

Assignees
No one assigned
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Local-Exec Issues and PRs for the Execution (Local) team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fmeum @larsrc-google