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

tar: Make all gzip compression hermetic #75

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

DolceTriade
Copy link
Contributor

@DolceTriade DolceTriade commented Aug 17, 2024

We weren't passing the flag to make gzip hermetic. Without the flag to
ignore timestamp, the shasum will differ based on the system time.

Also, DEFAULT_ARGS contained "--options=gzip:...", which is invalid when
using any other compression scheme. Therefore, remove that option from
DEFAULT_ARGS and provide an alternate function called
add_default_compression_args to add per-compression type default
arguments which can do things like make the compression more hermetic.

Repro instructions:

bazel test //...
find -L bazel-bin -name "*.gz" -exec sha256sum {} \; > sha
bazel clean --expunge
bazel test //...
sha256sum -c sha

Before, the above test would fail with sha256sum mismatches. With this patch, all shasums match.

Copy link
Collaborator

@thesayyn thesayyn left a comment

Choose a reason for hiding this comment

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

Thanks!

@DolceTriade
Copy link
Contributor Author

tar: Unknown module name: `gzip'

Hm, I thought that the aspect build tar would support this universally?

@DolceTriade DolceTriade force-pushed the hermetictar branch 2 times, most recently from 08cf7ce to 3e3b5ed Compare August 22, 2024 09:48
@DolceTriade DolceTriade changed the title flatten: Make flatten hermetic tar: Make all gzip compression hermetic Aug 22, 2024
@DolceTriade
Copy link
Contributor Author

Ok, tests pass now. While running I realized a lot more than flatten was not hermetic, so I fixed those as well. Please let me know if you'd like a different approach. I also updated the commit message with the motivations.

@jjmaestro
Copy link
Contributor

I'd love to get this merged, I'm also using flatten and will need the hermeticity! :)

@thesayyn
Copy link
Collaborator

@DolceTriade could you rebase and solve the merge conflicts? there are also some buildifier issues it seems

We weren't passing the flag to make gzip hermetic. Without the flag to
ignore timestamp, the shasum will differ based on the system time.

Also, DEFAULT_ARGS contained "--options=gzip:...", which is invalid when
using any other compression scheme. Therefore, remove that option from
DEFAULT_ARGS and provide an alternate function called
`add_default_compression_args` to add per-compression type default
arguments which can do things like make the compression more hermetic.
@DolceTriade
Copy link
Contributor Author

Done. @thesayyn PTAL, thanks!

@DolceTriade
Copy link
Contributor Author

I'm guessing the buildifier failures are due to differing versions of buildifier...

@DolceTriade
Copy link
Contributor Author

Hm, nvm, I didn't even modify the file in question?

@jjmaestro
Copy link
Contributor

Yeah, the buildifier errors are due to other PRs, I've fixed it in #108

@thesayyn thesayyn merged commit 7853bcd into GoogleContainerTools:main Oct 28, 2024
9 of 10 checks passed
@DolceTriade DolceTriade deleted the hermetictar branch October 28, 2024 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants