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

Jump to bottom

buildRustCrate: add lib output #73803

Merged
merged 1 commit into from Nov 28, 2019
Merged

buildRustCrate: add lib output #73803

merged 1 commit into from Nov 28, 2019

Conversation

andir
Copy link
Member

@andir 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
Copy link
Member Author

andir commented Nov 20, 2019

@GrahamcOfBorg build buildRustCrateTests

@andir
Copy link
Member Author

andir commented Nov 20, 2019

@GrahamcOfBorg build carnix

@ofborg ofborg bot added the 6.topic: rust label Nov 20, 2019
@ofborg ofborg bot requested a review from Gerschtli November 20, 2019 07:02
Ekleog
Ekleog approved these changes Nov 21, 2019
View reviewed changes
Copy link
Member

@Ekleog Ekleog left a comment

Choose a reason for hiding this comment

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

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
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.

jonringer
jonringer approved these changes Nov 24, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

This fixes #53026

@andir
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
buildRustCrate: add lib output
1b74855 
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
Copy link
Member Author

andir commented Nov 26, 2019

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

jonringer
jonringer approved these changes Nov 26, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

@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
Copy link
Member Author

andir commented Nov 27, 2019 •
edited

@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
Copy link
Contributor

not sure if darwin worked in the first place :/

@andir andir merged commit 059faab into NixOS:master Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ekleog Ekleog Ekleog approved these changes

@jonringer jonringer jonringer approved these changes

@Gerschtli Gerschtli Awaiting requested review from Gerschtli

Assignees
No one assigned
Labels
6.topic: nixos 6.topic: rust 8.has: changelog 8.has: documentation 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@andir @lopsided98 @jonringer @Ekleog