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

Fixes Windows symlinks targets #2239

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

claudiubelu
Copy link
Contributor

When building with the output type=oci or type=registry, for Windows symlinks, Files/ is prepended to the symlink Target, which can break the symlink entirely if it's an absolute path (C:\path\to\file becomes Files\C:\path\to\file), and for relative paths workarounds were needed (path\to\file would become Files\path\to\file, and it would have worked if that path was valid).

This problem does not occur when the output type is tar.

Removes the Files/ prefix from the Windows symlinks.

Fixes: docker/buildx#373

@tonistiigi
Copy link
Member

Probably C:/ needs a special case. Or I wonder if this should be done for symlinks at all. It needs to match the applier behavior and I'm not quite sure what actual wcow builders put in tarball for symlink/hardlink cases.

@claudiubelu claudiubelu force-pushed the fixes-windows-symlinks branch 2 times, most recently from e93d933 to ef2de1f Compare July 13, 2021 19:07
When building with the output type=oci or type=registry, for Windows
symlinks, "Files/" is prepended to the symlink Target, which can break
the symlink entirely if it's an absolute path (C:\path\to\file becomes
Files\C:\path\to\file), and for relative paths workarounds were needed
(path\to\file would become Files\path\to\file, and it would have worked
if that path was valid).

This problem does not occur when the output type is tar.

Removes the "Files/" prefix from the Windows symlinks.

Signed-off-by: Claudiu Belu <[email protected]>
@claudiubelu claudiubelu force-pushed the fixes-windows-symlinks branch from ef2de1f to 8cee9c7 Compare July 24, 2021 14:15
@claudiubelu
Copy link
Contributor Author

Probably C:/ needs a special case. Or I wonder if this should be done for symlinks at all. It needs to match the applier behavior and I'm not quite sure what actual wcow builders put in tarball for symlink/hardlink cases.

I've limited this PR to not add the Files/ prefix to symlinks, so the hardlinks behaviour remains unchanged (it wasn't an issue before).

@claudiubelu
Copy link
Contributor Author

claudiubelu commented Aug 4, 2021

Probably C:/ needs a special case. Or I wonder if this should be done for symlinks at all. It needs to match the applier behavior and I'm not quite sure what actual wcow builders put in tarball for symlink/hardlink cases.

It seems that the buildkit applier uses the containerd archive/tar applier [1], which doesn't trim the Files/ prefix, it just applies the symlink as is [2]. I don't think that we should trim the Files/ prefix in [2], so we can avoid the niche error case in which someone would actually want to symlink to a local folder called Files, something like foo.bar <===> Files/foo.bar. So, it seems it's up to how the images are built in the first place.

I have already mentioned in the PR message that the Files/ prefix is not added when the output type is tar.

[1]

if _, err := archive.Apply(ctx, root, rc2); err != nil {

[2]
if err := os.Symlink(hdr.Linkname, path); err != nil {

@cpuguy83
Copy link
Member

cpuguy83 commented Oct 5, 2021

What's the status on this? @tonistiigi Was there something you wanted to change here?

@tonistiigi
Copy link
Member

@claudiubelu So you think we should also remove the Files/ trim in applier? That sgtm although it would break existing images that were created with the Differ adding the prefix.

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.

Windows: buildx breaks symlinks when copying them
5 participants