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

Hardlinks are dereferenced in generated archives #1896

Merged
merged 6 commits into from
Dec 20, 2023

Conversation

jjbustamante
Copy link
Member

@jjbustamante jjbustamante commented Sep 11, 2023

Summary

This PR adds a fix to handle hardlinks included into a buildpack when pack buildpack package is executed.

Output

Before

> docker images | rg test-buildpack
test-buildpack                   latest                 210fe8ffb454   43 years ago    210MB

large-file and large-file2 references the same file with a hardlink, we can notice the size of the buildpack image is bigger than expected because the hardlink is lost during the image creation.

Using dive, we can see:

Screenshot 2023-09-11 at 2 48 35 PM

After

> docker images | rg test-buildpack
test-buildpack                latest              db42087024b4   43 years ago    105MB

After the fix is applied, the final image size is as expected not duplicating the files

Using dive, we can see:

Screenshot 2023-09-11 at 2 51 18 PM

Documentation

  • Should this change be documented?
    • Yes, see #___
    • No

Related

Resolves #1286

@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Sep 11, 2023
@github-actions github-actions bot added this to the 0.31.0 milestone Sep 11, 2023
@jjbustamante jjbustamante force-pushed the bugfix/jjbustamante/issue-1286 branch from f7c6c6e to 0b8c317 Compare September 11, 2023 19:32
}

_, err = tw.Write(buf)
_, err = io.Copy(tw, tr)
Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed this line could throw the same error reported here

@jjbustamante jjbustamante force-pushed the bugfix/jjbustamante/issue-1286 branch from 0b8c317 to 9f8044d Compare September 11, 2023 20:06
Copy link
Member

@natalieparellano natalieparellano left a comment

Choose a reason for hiding this comment

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

@jjbustamante it's cool to see this taking shape. I left a couple comments in case it's helpful

pkg/buildpack/buildpack.go Outdated Show resolved Hide resolved
pkg/archive/archive_windows.go Outdated Show resolved Hide resolved
pkg/archive/archive.go Outdated Show resolved Hide resolved
@jjbustamante jjbustamante force-pushed the bugfix/jjbustamante/issue-1286 branch from e3a6ac6 to 074967a Compare September 13, 2023 20:52
@jjbustamante jjbustamante modified the milestones: 0.32.0, 0.33.0 Oct 23, 2023
@jjbustamante jjbustamante modified the milestones: 0.33.0, 0.32.0 Oct 31, 2023
@jjbustamante jjbustamante added type/bug Issue that reports an unexpected behaviour. and removed type/enhancement Issue that requests a new feature or improvement. labels Nov 7, 2023
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Nov 8, 2023
@jjbustamante jjbustamante force-pushed the bugfix/jjbustamante/issue-1286 branch 4 times, most recently from 232de13 to 76fb460 Compare November 15, 2023 15:57
@jjbustamante jjbustamante removed the type/enhancement Issue that requests a new feature or improvement. label Nov 15, 2023
jjbustamante and others added 3 commits November 15, 2023 15:32
@jjbustamante jjbustamante force-pushed the bugfix/jjbustamante/issue-1286 branch from 76fb460 to 5f5f2af Compare November 15, 2023 20:33
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Nov 15, 2023
Signed-off-by: Juan Bustamante <[email protected]>
@jjbustamante jjbustamante marked this pull request as ready for review November 15, 2023 21:55
@jjbustamante jjbustamante requested a review from a team as a code owner November 15, 2023 21:55
@jjbustamante jjbustamante requested a review from a team as a code owner November 15, 2023 21:55
Copy link
Member

@natalieparellano natalieparellano left a comment

Choose a reason for hiding this comment

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

Nice work @jjbustamante - I left one comment/question about the test assertion, but this seems just about ready. Hats off to you for figuring out the Windows stuff.

testhelpers/tar_assertions.go Show resolved Hide resolved
pkg/buildpack/buildpack_test.go Outdated Show resolved Hide resolved
Signed-off-by: Juan Bustamante <[email protected]>
Copy link

codecov bot commented Dec 20, 2023

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (d9fddda) 79.69% compared to head (4738eb2) 79.64%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1896      +/-   ##
==========================================
- Coverage   79.69%   79.64%   -0.05%     
==========================================
  Files         172      174       +2     
  Lines       13046    13126      +80     
==========================================
+ Hits        10396    10453      +57     
- Misses       1995     2010      +15     
- Partials      655      663       +8     
Flag Coverage Δ
os_linux 78.54% <77.50%> (+0.02%) ⬆️
os_macos 76.36% <77.50%> (-<0.01%) ⬇️
os_windows 79.03% <66.67%> (+0.55%) ⬆️
unit 79.64% <69.77%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@jjbustamante jjbustamante merged commit 7b5164c into main Dec 20, 2023
18 checks passed
@jjbustamante jjbustamante deleted the bugfix/jjbustamante/issue-1286 branch December 20, 2023 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Issue that reports an unexpected behaviour. type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hardlinks are dereferenced in generated archives increasing buildpack and app image sizes
2 participants