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

fix: Make artifact bundle creation deterministic #1652

Merged
merged 5 commits into from
Jun 22, 2023

Conversation

loewenheim
Copy link
Contributor

Building an artifact bundle is currently nondeterministic. This is because of two factors:

  1. We depend on an outdated version of symbolic in which the order in which you add files to a source bundle matters.
  2. When building the bundle, we iterate over source files in nondeterministic order.

This adds a failing test and then fixes the problem by updating symbolic and presorting source files.

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

Well the add_file API is still order dependent thats why you have to sort.

It used to use a default timestamp which was obviously not deterministic, but now we are just writing files with a deterministic timestamp. Looks like I never pushed that fix / update to symbolic. I remember I did try it locally for sure.

src/utils/file_upload.rs Outdated Show resolved Hide resolved
@loewenheim
Copy link
Contributor Author

Well the add_file API is still order dependent thats why you have to sort.

It used to use a default timestamp which was obviously not deterministic, but now we are just writing files with a deterministic timestamp. Looks like I never pushed that fix / update to symbolic. I remember I did try it locally for sure.

Should we fix that in symbolic before merging this or just leave it like it is?

@Swatinem
Copy link
Member

Well you also don’t want to buffer everything in RAM before you write it out to file… I guess that is one of the main reasons that add_file does not internally buffer/sort.

@loewenheim loewenheim merged commit 73010e6 into master Jun 22, 2023
@loewenheim loewenheim deleted the fix/artifact-bundle-determinism branch June 22, 2023 10:38
loewenheim added a commit that referenced this pull request Jun 26, 2023
loewenheim added a commit that referenced this pull request Jun 26, 2023
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