Skip to content

buildRustCrate: make codegen-units configurable#132721

Closed
Fuuzetsu wants to merge 1 commit intoNixOS:masterfrom
Fuuzetsu:rust-configurable-codegen-units
Closed

buildRustCrate: make codegen-units configurable#132721
Fuuzetsu wants to merge 1 commit intoNixOS:masterfrom
Fuuzetsu:rust-configurable-codegen-units

Conversation

@Fuuzetsu
Copy link
Copy Markdown
Member

@Fuuzetsu Fuuzetsu commented Aug 5, 2021

This parameter is being set to $NIX_BUILD_CORES by default. This is a
standard practice but there's a suspicion that this can produce broken
builds. For some details see
cargo2nix/cargo2nix#184 . As a
work-around/test, it'd be good if codegen-units can be set to something
constant, such as 1. This PR allows it.

Note that the default of $NIX_BUILD_CORES is preserved so this MR
causes no change in default behaviour and no rebuilds.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Relase notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. label Aug 5, 2021
@Fuuzetsu
Copy link
Copy Markdown
Member Author

Fuuzetsu commented Aug 5, 2021

Ah, I made a mistake in the PR and broke eval, I will fix it.

@Fuuzetsu Fuuzetsu force-pushed the rust-configurable-codegen-units branch from 5e65c3b to 91f0392 Compare August 5, 2021 02:24
@ofborg ofborg bot requested a review from adisbladis August 5, 2021 02:36
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Aug 5, 2021
@Fuuzetsu
Copy link
Copy Markdown
Member Author

Fuuzetsu commented Sep 13, 2021

Ping?

@danieldk danieldk removed their request for review September 13, 2021 11:46
@Fuuzetsu Fuuzetsu force-pushed the rust-configurable-codegen-units branch from 91f0392 to 84fd1f5 Compare October 12, 2021 00:24
@Fuuzetsu
buildRustCrate: make codegen-units configurable
1d5bca0 
This parameter is being set to `$NIX_BUILD_CORES` by default. This is a
standard practice but there's a suspicion that this can produce broken
builds. For some details see
cargo2nix/cargo2nix#184 . As a
work-around/test, it'd be good if codegen-units can be set to something
constant, such as `1`. This PR allows it.

Note that the default of `$NIX_BUILD_CORES` is preserved so this MR
causes no change in default behaviour and no rebuilds.
@Fuuzetsu Fuuzetsu force-pushed the rust-configurable-codegen-units branch from 84fd1f5 to 1d5bca0 Compare October 12, 2021 00:25
@Fuuzetsu
Copy link
Copy Markdown
Member Author

Fuuzetsu commented Oct 12, 2021

I've rebased on master... Adding extra reviewer based on file edit history...

@Fuuzetsu Fuuzetsu requested a review from Ericson2314 October 12, 2021 00:26
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. labels Oct 12, 2021
@Fuuzetsu
Copy link
Copy Markdown
Member Author

Fuuzetsu commented Oct 15, 2021

FWIW I determined that codegen-units=1 doesn't solve the build reproducibility issue which is sad. It'd still be nice to have this change included though as setting this value is useful anyway.

@Ericson2314
Copy link
Copy Markdown
Member

Ericson2314 commented Oct 15, 2021 •
edited
Loading

Could use make it instead use enableParallelBuilding, like https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/haskell-modules/generic-builder.nix#L111 ?

I don't super like the default of this being "$NIX_BUILD_CORES" because it's its kinda like advertising we can "bash inject", ourselves haha.

@stale
Copy link
Copy Markdown

stale bot commented Apr 16, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 16, 2022
@davidscherer
Copy link
Copy Markdown
Contributor

davidscherer commented Apr 30, 2022

Setting codegen-units=1 does fix a serious nondeterminism in Rust builds - #130309. I was about to make a PR for that, but it would be better to just change this one to default to 1 instead of $NIX_BUILD_CORES. (Probably any fixed setting is deterministic, but I think 1 produces the most efficient binaries according to the Rust docs.)

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 30, 2022
@davidscherer davidscherer mentioned this pull request Apr 30, 2022
Merged
9 tasks
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 30, 2022
@Fuuzetsu
Copy link
Copy Markdown
Member Author

Fuuzetsu commented Nov 3, 2022

Closing this as codegenUnits can be set for each crate now. We've been using 8 for a while.

@Fuuzetsu Fuuzetsu closed this Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andir andir Awaiting requested review from andir

@zowoq zowoq Awaiting requested review from zowoq

@adisbladis adisbladis Awaiting requested review from adisbladis

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

Assignees

No one assigned

Labels

2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Fuuzetsu @Ericson2314 @davidscherer