[9.0.1] Introduce RunfilesProxyArtifactValue.#28516
[9.0.1] Introduce RunfilesProxyArtifactValue.#28516iancha1992 merged 1 commit intobazelbuild:release-9.0.1from
Conversation
This specialization of FileArtifactValue is now returned by RunfilesArtifactValue.getMetadata(), thereby ensuring that the metadata is of directory type. This is required for it to be handled correctly by the BEP. This is intended to be a low-risk targeted fix to be cherry-picked into 9.x. It leaves a couple of open questions to be addressed: * Should RunfilesArtifactValue itself implement FileArtifactValue and avoid the extra indirection? (The impact on memory usage is unclear.) * Should the SpecialArtifact for a runfiles tree also return true from isDirectory()? (It may require fixing callers that only expect it to return true for tree artifacts and filesets.) Fixes bazelbuild#28330. PiperOrigin-RevId: 865346372 Change-Id: I49c897ca444547b077c1d471b7ac670bd83bae34
There was a problem hiding this comment.
Code Review
This pull request introduces RunfilesProxyArtifactValue, a specialization of FileArtifactValue, to ensure that metadata for runfiles trees is correctly identified as a directory type. This is a targeted fix to address an issue with the Build Event Protocol (BEP). The change is well-implemented, introducing a new private inner class that correctly reports its type as DIRECTORY while being identified by a content digest. The related changes, including renaming createProxy to the more specific createRunfilesProxy and updating a test to use a more appropriate method, are also correct and improve code clarity. The changes are clean, minimal, and effectively solve the stated problem. The implementation is solid and I have no concerns.
This specialization of FileArtifactValue is now returned by RunfilesArtifactValue.getMetadata(), thereby ensuring that the metadata is of directory type. This is required for it to be handled correctly by the BEP.
This is intended to be a low-risk targeted fix to be cherry-picked into 9.x. It leaves a couple of open questions to be addressed:
Should RunfilesArtifactValue itself implement FileArtifactValue and avoid the extra indirection? (The impact on memory usage is unclear.)
Should the SpecialArtifact for a runfiles tree also return true from isDirectory()? (It may require fixing callers that only expect it to return true for tree artifacts and filesets.)
Fixes #28330.
PiperOrigin-RevId: 865346372
Change-Id: I49c897ca444547b077c1d471b7ac670bd83bae34
Commit 952aabb