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

Write out manifest.in entries deterministically #120

Merged
merged 1 commit into from
Feb 4, 2023

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Feb 4, 2023

Currently, the order of the entries in manifest.in depends on the iteration order of copy_with_callback, which in turn depends on WalkDir, which explicitly says iteration order is unspecified. It's possible to call WalkDir::sort_by to give an iteration order for each directory's entries, but it felt better to accumulate the lines and then sort them to a) make it more evident that this is happening, and b) enable the copying to be parallelized in the future.

Currently, the order of the entries in `manifest.in` depends on the
iteration order of `copy_with_callback`, which in turn depends on
`WalkDir`, which explicitly says iteration order is unspecified. It's
possible to call `WalkDir::sort_by` to give an iteration order for each
directory's entries, but it felt better to accumulate the lines and then
sort them to a) make it more evident that this is happening, and b)
enable the copying to be parallelized in the future.
@rustbot
Copy link

rustbot commented Feb 4, 2023

Failed to set assignee to Mark-Simulacrum: cannot assign: HTTP status client error (403 Forbidden) for url (https://api.github.com/repos/rust-lang/rust-installer/issues/120/assignees)

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

@rustbot
Copy link

rustbot commented Feb 4, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

We'll want a PR against rust-lang/rust bumping the submodule there to get these changes into releases.

@Mark-Simulacrum Mark-Simulacrum merged commit 9981e4d into rust-lang:master Feb 4, 2023
@jonhoo jonhoo deleted the deterministic-manifest branch February 4, 2023 01:55
@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 4, 2023

Done! rust-lang/rust#107656

bors pushed a commit to rust-lang-ci/rust that referenced this pull request Feb 4, 2023
Makes generation of `manifest.in` deterministic:
rust-lang/rust-installer#120
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 8, 2023
…-Simulacrum

Bump rust-installer

Makes generation of `manifest.in` deterministic:
rust-lang/rust-installer#120
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 8, 2023
…-Simulacrum

Bump rust-installer

Makes generation of `manifest.in` deterministic:
rust-lang/rust-installer#120
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