Skip to content

dap: add ability to keep ops unique when marshaling - #6119

Closed
jsternberg wants to merge 1 commit into
moby:masterfrom
jsternberg:dap-unique-identities
Closed

jsternberg wants to merge 1 commit into
moby:masterfrom
jsternberg:dap-unique-identities

Conversation

@jsternberg

Copy link
Copy Markdown
Collaborator

Marshaling with the WithIdentities() constraint will ensure operations
are given unique identities to ensure they marshal with different
values. This prevents the same operation from being merged with other
identical operations.

This is useful for the debug adapter protocol which wants to know the
original locations of operations when they are in unique places.

This behavior can be enabled for dockerfiles by setting
BUILDKIT_WITH_IDENTITIES.

Marshaling with the `WithIdentities()` constraint will ensure operations
are given unique identities to ensure they marshal with different
values. This prevents the same operation from being merged with other
identical operations.

This is useful for the debug adapter protocol which wants to know the
original locations of operations when they are in unique places.

This behavior can be enabled for dockerfiles by setting
`BUILDKIT_WITH_IDENTITIES`.

Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
@jsternberg
jsternberg requested a review from tonistiigi August 5, 2025 16:20

@tonistiigi tonistiigi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this is the right approach. It is beneficial for the solver to have matching LLB digest as this is the most optimal way how the solver can deduplicate the build graph. Even if the cache keys still match, the deduplication would be added in the "merging edges" stage, that is a much more complicated codepath.

I think if we want to add more debug info to the steps, eg. that one step exists in multiple places of the source code, we should do it outside of the object that defines LLB digest, like how sourcemap is handled atm. Iirc even the name of the step does not participate in the LLB digest for that reason, so we shouldn't do it for extra debug info.

@thompson-shaun thompson-shaun added this to the v0.24.0 milestone Aug 21, 2025
@crazy-max crazy-max removed this from the v0.24.0 milestone Aug 27, 2025
@jsternberg
jsternberg marked this pull request as draft September 15, 2025 15:50
@jsternberg

Copy link
Copy Markdown
Collaborator Author

Closing this for now. I'm not a big fan of this solution and we were able to work around the issue with this PR inside of buildx. I'll keep the code around just in case that solution isn't good enough, but I would prefer not adding a hacky solution like this to the LLB if we don't really need it.

@jsternberg jsternberg closed this Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants