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.

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

Fix LocalFileType computation when the artifact and its metadata disagree. #20419

Closed
wants to merge 1 commit into from

Conversation

tjgq
Copy link
Contributor

@tjgq tjgq commented Dec 3, 2023

When a tree artifact contains a symlink to a directory, the TreeArtifactValue contains a TreeFileArtifact mapping to a DirectoryArtifactValue (see #20418).

Because the file type to be uploaded to the BES is determined solely by the Artifact type, this causes the uploader to attempt to upload a directory as if it were a file. This fails silently when building with the bytes; when building without the bytes, it crashes when attempting to digest the (in-memory) directory, which has no associated inode (see #20415).

This PR fixes the file type computation to take into account both the artifact and its metadata, preferring the latter when available.

Fixes #20415.

@tjgq tjgq requested a review from meisterT December 3, 2023 16:28
@tjgq tjgq changed the title Fix LocalFileType for symlink to directory inside tree artifact. Fix LocalFileType computation when the artifact and its metadata disagree. Dec 3, 2023
@tjgq tjgq force-pushed the tree-with-sym-to-dir-fix branch from 1e44dcb to e50eb0c Compare December 3, 2023 16:59
@tjgq tjgq marked this pull request as ready for review December 3, 2023 16:59
@tjgq tjgq requested a review from a team as a code owner December 3, 2023 16:59
@tjgq tjgq requested review from aiuto and removed request for a team December 3, 2023 16:59
@github-actions github-actions bot added team-Performance Issues for Performance teams team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels Dec 3, 2023
@tjgq tjgq force-pushed the tree-with-sym-to-dir-fix branch 2 times, most recently from 86088e8 to 370768c Compare December 3, 2023 21:17
…gree.

When a tree artifact contains a symlink to a directory, the TreeArtifactValue
contains a TreeFileArtifact mapping to a DirectoryArtifactValue (see bazelbuild#20418).

Because the file type to be uploaded to the BES is determined solely by the
Artifact type, this causes the uploader to attempt to upload a directory as if
it were a file. This fails silently when building with the bytes; when building
without the bytes, it crashes when attempting to digest the (in-memory)
directory, which has no associated inode (see bazelbuild#20415).

This PR fixes the file type computation to take into account both the artifact
and its metadata, preferring the latter when available.

Fixes bazelbuild#20415.
@tjgq tjgq force-pushed the tree-with-sym-to-dir-fix branch from 370768c to 17f085d Compare December 3, 2023 21:18
@copybara-service copybara-service bot closed this in bb5ff63 Dec 4, 2023
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Dec 4, 2023
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Dec 4, 2023
…gree.

When a tree artifact contains a symlink to a directory, the TreeArtifactValue contains a TreeFileArtifact mapping to a DirectoryArtifactValue (see bazelbuild#20418).

Because the file type to be uploaded to the BES is determined solely by the Artifact type, this causes the uploader to attempt to upload a directory as if it were a file. This fails silently when building with the bytes; when building without the bytes, it crashes when attempting to digest the (in-memory) directory, which has no associated inode (see bazelbuild#20415).

This PR fixes the file type computation to take into account both the artifact and its metadata, preferring the latter when available.

Fixes bazelbuild#20415.

Closes bazelbuild#20419.

PiperOrigin-RevId: 587654070
Change-Id: Ib62bbaaed62b00c12bcabdc2bc9bee57aee0bcef
keertk pushed a commit that referenced this pull request Dec 4, 2023
…act containing symlink to directory + remote cache + BES (#20424)

When a tree artifact contains a symlink to a directory, the
TreeArtifactValue contains a TreeFileArtifact mapping to a
DirectoryArtifactValue (see #20418).

Because the file type to be uploaded to the BES is determined solely by
the Artifact type, this causes the uploader to attempt to upload a
directory as if it were a file. This fails silently when building with
the bytes; when building without the bytes, it crashes when attempting
to digest the (in-memory) directory, which has no associated inode (see
#20415).

This PR fixes the file type computation to take into account both the
artifact and its metadata, preferring the latter when available.

Fixes #20415.

Closes #20419.

Commit
bb5ff63

PiperOrigin-RevId: 587654070
Change-Id: Ib62bbaaed62b00c12bcabdc2bc9bee57aee0bcef

Co-authored-by: Tiago Quelhas <tjgq@google.com>
@tjgq tjgq deleted the tree-with-sym-to-dir-fix branch March 7, 2024 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions team-Performance Issues for Performance teams
Projects
None yet
2 participants