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

buildRustCrate: add lib output #73803

Merged
merged 1 commit into from Nov 28, 2019
Merged

Conversation

@andir
Copy link
Member

andir commented Nov 20, 2019

Motivation for this change

This cuts down the dependency tree on some rust builds where a crate not
just exposes a binary but also a library. $out/lib contained a bunch
of extra support files that among other information carry linker flags
(including the full path to link-time dependencies). Worst case this led
to some binary outputs depending on the full build closure of rust
crates.

Moving all the $out/lib files to $lib/lib solves this nicely.

lib might be a bit weird here as they are most of the time just rlib
files (rust libraries). Those are essential only required during
compilation but they can also be shared objects (like with traditional
C-style packages). Which is why I went with lib for the new output.

One of the caveats we are running into here is that we do not (always)
know ahead of time of a crate produces just a library or just a binary.
Cargo allows for some ambiguity regarding whether or not a crate
provides one, two, … binaries and libraries as it's outputs. Ideally we
would be able to rely on the crateType entirely but so far that isn't
the case. More work on that area might show how difficult that actually
is.

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 nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @Mic92

@andir

This comment has been minimized.

Copy link
Member Author

andir commented Nov 20, 2019

@GrahamcOfBorg build buildRustCrateTests

@andir

This comment has been minimized.

Copy link
Member Author

andir commented Nov 20, 2019

@GrahamcOfBorg build carnix

@Ekleog
Ekleog approved these changes Nov 21, 2019
Copy link
Member

Ekleog left a comment

Do we have a way to list all the rust packages in hydra? I don't know about one, but it'd be nice to be able to check that landing this PR doesn't dramatically increase the number of failures.

Also, this probably needs release notes.

@andir

This comment has been minimized.

Copy link
Member Author

andir commented Nov 23, 2019

Do we have a way to list all the rust packages in hydra? I don't know about one, but it'd be nice to be able to check that landing this PR doesn't dramatically increase the number of failures.

Yes we do. ofBorg did do that check for us already: https://gist.githubusercontent.com/GrahamcOfBorg/65e023463bccb03ab9f479057707a161/raw/7b6376dd8a0814a98cfff827cb0fd24e20c6b716/Changed%2520Paths

And these are all those that are relevant:
@GrahamcOfBorg build toml2nix carnix cargo-update cargo-download buildRustCrateTests

Also, this probably needs release notes.

What kind of notes do you like to see for this? We have a bit of (outdated?) buildRustCrate documentation that needs to be fixed but that is probably out of scope for this change. We could state that we are now splitting the builds and everything is much better… Not sure if that is of much value but could be documented. From a user perspective there is not really a change.

Copy link
Contributor

jonringer left a comment

nix-review passes on NixOS

[310 built, 249 copied (114.3 MiB), 14.7 MiB DL]
https://github.com/NixOS/nixpkgs/pull/73803
1 package are marked as broken and were skipped:
way-cooler

18 package were built:
buildRustCrateTests.allocNoStdLibTest buildRustCrateTests.brotliDecompressorTest buildRustCrateTests.brotliTest buildRustCrateTests.crateBinNoPath1 buildRustCrateTests.crateBinNoPath2 buildRustCrateTests.crateBinNoPath3 buildRustCrateTests.crateBinNoPath4 buildRustCrateTests.crateBinRename1 buildRustCrateTests.crateBinRename2 buildRustCrateTests.crateBinWithPath buildRustCrateTests.customLibName buildRustCrateTests.customLibNameAndLibPath buildRustCrateTests.libPath buildRustCrateTests.srcLib cargo-download cargo-update carnix toml2nix

would like for someone to do some more in-depth testing

@lopsided98

This comment has been minimized.

Copy link
Contributor

lopsided98 commented Nov 25, 2019

This fixes #53026

@andir

This comment has been minimized.

Copy link
Member Author

andir commented Nov 26, 2019

@jonringer I did build https://broken.sh, a clients code base and a few of my other private projects using this. So far it really looks good. Let me know if you still prefer someone else having a look at this. It would probably be sane.

@andir andir force-pushed the andir:buildRustCrate-lib-output branch from bb8dd63 to acb3b20 Nov 26, 2019
This cuts down the dependency tree on some rust builds where a crate not
just exposes a binary but also a library. `$out/lib` contained a bunch
of extra support files that among other information carry linker flags
(including the full path to link-time dependencies). Worst case this led
to some binary outputs depending on the full build closure of rust
crates.

Moving all the `$out/lib` files to `$lib/lib` solves this nicely.

`lib` might be a bit weird here as they are most of the time just rlib
files (rust libraries). Those are essential only required during
compilation but they can also be shared objects (like with traditional
C-style packages). Which is why I went with `lib` for the new output.

One of the caveats we are running into here is that we do not (always)
know ahead of time of a crate produces just a library or just a binary.
Cargo allows for some ambiguity regarding whether or not a crate
provides one, two, … binaries and libraries as it's outputs. Ideally we
would be able to rely on the `crateType` entirely but so far that isn't
the case. More work on that area might show how difficult that actually
is.
@andir andir force-pushed the andir:buildRustCrate-lib-output branch from acb3b20 to 1b74855 Nov 26, 2019
@andir

This comment has been minimized.

Copy link
Member Author

andir commented Nov 26, 2019

@Ekleog I added some basic changelog lines. Let me know if that is good (enough).

Copy link
Contributor

jonringer left a comment

LGTM

[310 built, 249 copied (114.3 MiB), 14.7 MiB DL]
https://github.com/NixOS/nixpkgs/pull/73803
1 package are marked as broken and were skipped:
way-cooler

18 package were built:
buildRustCrateTests.allocNoStdLibTest buildRustCrateTests.brotliDecompressorTest buildRustCrateTests.brotliTest buildRustCrateTests.crateBinNoPath1 buildRustCrateTests.crateBinNoPath2 buildRustCrateTests.crateBinNoPath3 buildRustCrateTests.crateBinNoPath4 buildRustCrateTests.crateBinRename1 buildRustCrateTests.crateBinRename2 buildRustCrateTests.crateBinWithPath buildRustCrateTests.customLibName buildRustCrateTests.customLibNameAndLibPath buildRustCrateTests.libPath buildRustCrateTests.srcLib cargo-download cargo-update carnix toml2nix
@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Nov 26, 2019

@GrahamcOfBorg build buildRustCrateTests.allocNoStdLibTest buildRustCrateTests.brotliDecompressorTest buildRustCrateTests.brotliTest buildRustCrateTests.crateBinNoPath1 buildRustCrateTests.crateBinNoPath2 buildRustCrateTests.crateBinNoPath3 buildRustCrateTests.crateBinNoPath4 buildRustCrateTests.crateBinRename1 buildRustCrateTests.crateBinRename2 buildRustCrateTests.crateBinWithPath buildRustCrateTests.customLibName buildRustCrateTests.customLibNameAndLibPath buildRustCrateTests.libPath buildRustCrateTests.srcLib cargo-download cargo-update carnix toml2nix

@andir

This comment has been minimized.

Copy link
Member Author

andir commented Nov 27, 2019

@GrahamcOfBorg build buildRustCrateTests.allocNoStdLibTest buildRustCrateTests.brotliDecompressorTest buildRustCrateTests.brotliTest buildRustCrateTests.crateBinNoPath1 buildRustCrateTests.crateBinNoPath2 buildRustCrateTests.crateBinNoPath3 buildRustCrateTests.crateBinNoPath4 buildRustCrateTests.crateBinRename1 buildRustCrateTests.crateBinRename2 buildRustCrateTests.crateBinWithPath buildRustCrateTests.customLibName buildRustCrateTests.customLibNameAndLibPath buildRustCrateTests.libPath buildRustCrateTests.srcLib cargo-download cargo-update carnix toml2nix

Rerunning due to darwin failure. Not sure if that is flaky on that strange platform.

@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Nov 27, 2019

not sure if darwin worked in the first place :/

@andir andir merged commit 059faab into NixOS:master Nov 28, 2019
16 checks passed
16 checks passed
buildRustCrateTests.allocNoStdLibTest, buildRustCrateTests.brotliDecompressorTest, buildRustCrateTests.brotliTest, buildRustCrateTests.crateBinNoPath1, buildRustCrateTests.crateBinNoPath2, buildRustCrateTests.crateBinNoPath3, buildRustCrateTests.crateBinNoPath4, buildRustCrateTests.crateBinRename1, buildRustCrateTests.crateBinRename2, buildRustCrateTests.crateBinWithPath, buildRustCrateTests.customLibName, buildRustCrateTests.customLibNameAndLibPath, buildRustCrateTests.libPath, buildRustCrateTests.srcLib, cargo-download, cargo-update, carnix, toml2nix on x86_64-darwin Failure
Details
Evaluation Performance Report Evaluator Performance Report
Details
buildRustCrateTests.allocNoStdLibTest, buildRustCrateTests.brotliDecompressorTest, buildRustCrateTests.brotliTest, buildRustCrateTests.crateBinNoPath1, buildRustCrateTests.crateBinNoPath2, buildRustCrateTests.crateBinNoPath3, buildRustCrateTests.crateBinNoPath4, buildRustCrateTests.crateBinRename1, buildRustCrateTests.crateBinRename2, buildRustCrateTests.crateBinWithPath, buildRustCrateTests.customLibName, buildRustCrateTests.customLibNameAndLibPath, buildRustCrateTests.libPath, buildRustCrateTests.srcLib, cargo-download, cargo-update, carnix, toml2nix on aarch64-linux Success
Details
buildRustCrateTests.allocNoStdLibTest, buildRustCrateTests.brotliDecompressorTest, buildRustCrateTests.brotliTest, buildRustCrateTests.crateBinNoPath1, buildRustCrateTests.crateBinNoPath2, buildRustCrateTests.crateBinNoPath3, buildRustCrateTests.crateBinNoPath4, buildRustCrateTests.crateBinRename1, buildRustCrateTests.crateBinRename2, buildRustCrateTests.crateBinWithPath, buildRustCrateTests.customLibName, buildRustCrateTests.customLibNameAndLibPath, buildRustCrateTests.libPath, buildRustCrateTests.srcLib, cargo-download, cargo-update, carnix, toml2nix on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.