cargo-vendor: Build from source using carnix #33980

thefloweringash · 2018-01-17T15:17:56Z

Motivation for this change

Build cargo-vendor from source #32254

So I can try to build firefox on aarch64.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
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 nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

I had a go at building cargo-vendor using carnix and buildRustCrate. There are a few awkward things here I'd like to tidy, flagged with "TODO" in the source.

crateOverrides in the documentation suggests that you can override any attribute, with an example for postPatch, but my preBuild hook was ignored. I very crudely added it, but I don't know how it should work.
The inference for the source file for a [[bin]] section in a Cargo.toml file doesn't seem to match cargo. I couldn't find it in the documentation, but the tests suggest that the expected location for foo is src/bin/foo.rs, while accepting src/main.rs with a warning. However in buildRustCrate, only src/main.rs is checked.
cargo-vendor itself needs to emit a binary called cargo-vendor to be a cargo subcommand, but it defaults to cargo_vendor.
The src attribute for the main target is not optional when using carnix, defaulting to ./.. The crate we're building cargo-vendor, is in crates.io, and could be fetched with the same logic as the rest of the dependencies. What's preferred here?

Mic92 · 2018-01-17T15:44:44Z

cc @P-E-Meunier

Mic92 · 2018-01-17T15:49:59Z

pkgs/build-support/rust/build-rust-crate.nix

@@ -332,6 +332,9 @@ stdenv.mkDerivation rec {
    libName = if crate ? libName then crate.libName else crate.crateName;
    libPath = if crate ? libPath then crate.libPath else "";

+    # TODO: the documentation suggests this works, is this necessary?
+    preBuild = if lib.hasAttr "preBuild" crate then crate.preBuild else "";


We need to rethink, how crateOverrides are applied here.

Ideally it should work the same way as defaultGemConfig: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/ruby-modules/bundled-common/default.nix#L38

we would need to pass all arguments to the function defined by cratesOverrides before passing it to the underlying stdenv.mkDerivation

If I remember correctly, my intention was to allow the user to override the derivation with standard Nix functions, but having multiple ways of doing this is not too bad. Wouldn't it look better with an inherit of multiple attributes instead of this extra line?

I want this to configure the build of one of cargo-vendor's dependencies: libgit2-sys. I don't see how to use the standard Nix functions in this case. If I use overrideAttrs, I can produce a derivation for the libgit2-sys that will build but it wouldn't be used as a dependency, since the dependencies are assembled by carnix as part of a rec attribute map. If this were configurable like the overall package set, then it would be possible.

Even if it were possible to change the dependency using the standard methods, it seems like this is what crateOverrides are for. Any package using libgit2-sys requires this configuration, so crateOverrides should be able to specify more build attributes. I can do the fairly simple thing of mkDerivation ({ ... } // crate), (removing things already processed) or I can whitelist particular attributes, or even make an extra attribute in crateOverrides for this purpose. Which one would you prefer?

mkDerivation ({ ... } // crate) sounds like what I would expect from crateOverrides.
And we definitely need access to the patch phase in crateOverrides.

P-E-Meunier · 2018-01-17T18:27:44Z

pkgs/build-support/rust/cargo-vendor/default.nix

+  inherit (darwin.apple_sdk.frameworks) CoreFoundation;
+in
+
+(callPackage ./cargo-vendor.nix {}).cargo_vendor_0_1_13.override {


This can indeed be used in this way, but the intention is to gather all possible crateOverrides in pkgs/build-support/rust/default-crate-overrides.nix instead. The goal is that other users of the same packages won't have to do this again.

This mechanism is indented for overrides outside of nixpkgs.

P-E-Meunier · 2018-01-17T18:28:34Z

Cool! Thanks for trying carnix out.

Removes a binary bootstrap, and enables cargo-vendor on aarch64.

dezgeg · 2018-01-19T07:49:24Z

Yay, I've tested this successfully builds firefox on aarch64. I had to add this:

diff --git a/pkgs/development/compilers/rust/cargo.nix b/pkgs/development/compilers/rust/cargo.nix
index fb3001bbf17..36f55dc19f5 100644
--- a/pkgs/development/compilers/rust/cargo.nix
+++ b/pkgs/development/compilers/rust/cargo.nix
@@ -63,6 +63,6 @@ rustPlatform.buildRustPackage rec {
     description = "Downloads your Rust project's dependencies and builds your project";
     maintainers = with maintainers; [ wizeman retrry ];
     license = [ licenses.mit licenses.asl20 ];
-    platforms = [ "x86_64-linux" "x86_64-darwin" ];
+    platforms = platforms.unix;
   };
 }

or similar.

thefloweringash · 2018-01-19T08:07:50Z

There's an interesting quirk to this package. As far as I can tell, it's only used to produce fixed output derivations which are typically present in the cache. Removing the assertion failure was sufficient to build (and run!) firefox on aarch64, (though apparently I had allowBroken in my global config, so didn't notice the platforms mask).

Mic92 · 2018-01-19T10:10:12Z

pkgs/build-support/rust/build-rust-crate.nix

    buildInputs_ = buildInputs;
 in
-stdenv.mkDerivation rec {
+stdenv.mkDerivation (rec {


the following can be also simplified then:

- buildInputs = [ rust ncurses ] ++ (crate.buildInputs or []) ++ buildInputs_; + buildInputs = [ rust ncurses ] ++ buildInputs_;

As is, I don't think this is possible. If the crateOverrides includes buildInputs, that will merged on top of the attribute passed set to mkDerivation then the rust and ncurses inputs will be lost. We could rename the attribute in crate overrides to be extraBuildInputs to be less surprising, or use a different composition that lets crateOverrides read more of the context it's updating, so it can explicitly append or replace the value as appropriate.

Forgot about that. You are right.

dezgeg · 2018-01-19T10:34:31Z

Ah, good point, indeed it's used for fixed-output derivations. But forcing a nix-build --check indeed shows it works on aarch64.

Mic92 · 2018-01-19T12:07:59Z

pkgs/build-support/rust/build-rust-crate.nix

+      "buildDependencies" "dependencies" "features"
+      "crateName" "version" "build" "authors" "colors"
+    ];
+    extraDerivationAttrs = lib.filterAttrs (n: v: ! lib.elem n processedAttrs) crate;


Rather then filtering I would make an assertion to make debugging easier, in case somebody tries to set them.
Or is there any use case for that?

The intent here was to remove only the attributes that had a special interpretation in the body below. All of these are valid to set. Re-reading, I think I could simplify colors, build, libName and libPath.

Yes, I see that. But users might try to override them in cratesOverrides and wonder why the override has no effect for these special attributes. And assertion would make this an runtime error instead.

When users set these fields in crateOverrides, the overrides become part of crate, and will have some meaning in the body. They're not being silently ignored. I agree it's potentially confusing that setting crateBin doesn't pass through without being transformed, or that features is renamed and transformed to crateFeatures, but they do make it through in some expression.

ok. Then we it should be kept this way.

Mic92 · 2018-01-19T12:08:41Z

Apart from my last comment, this pull request is then good to go.

bachp · 2018-01-19T18:04:19Z

Not sure if this helps here, but there is a source tarball for cargo vendor on the release page that has all the dependencies vendored in.

It's called: cargo-vendor-src-${version}.tar.gz. The idea was to allow build cargo-vendor without requiering cargo-vendor to fetch the dependencies first.

The tarball is generated on every release automatically by CI. See alexcrichton/cargo-vendor#51 for more details.

Mic92 · 2018-01-19T23:02:31Z

This would work too. But the advantage of carnix is that we can also replace our bootstrap cargo binary and build cargo directly from source instead.

Mic92 · 2018-01-21T10:51:41Z

@GrahamcOfBorg build cargo-vendor

GrahamcOfBorg

Failure for system: x86_64-linux

error: attribute ‘cargo-vendor’ in selection path ‘cargo-vendor’ not found

GrahamcOfBorg

Failure for system: aarch64-linux

error: attribute 'cargo-vendor' in selection path 'cargo-vendor' not found

Mic92 · 2018-01-21T10:53:28Z

@LnL7 could you test this pr on darwin sometime?

grahamc · 2018-01-21T12:48:26Z

@GrahamcOfBorg build rust.cargo-vendor

GrahamcOfBorg

Failure for system: x86_64-linux

error: attribute ‘cargo-vendor’ in selection path ‘rust.cargo-vendor’ not found

GrahamcOfBorg

Failure for system: aarch64-linux

error: attribute 'cargo-vendor' in selection path 'rust.cargo-vendor' not found

GrahamcOfBorg

Failure for system: x86_64-darwin

error: attribute 'cargo-vendor' in selection path 'cargo-vendor' not found

GrahamcOfBorg

Failure for system: x86_64-darwin

error: attribute 'cargo-vendor' in selection path 'rust.cargo-vendor' not found

thefloweringash changed the title ~~Cargo vendor carnix~~ cargo-vendor: Build from source using carnix Jan 17, 2018

GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jan 17, 2018

Mic92 reviewed Jan 17, 2018

View reviewed changes

P-E-Meunier reviewed Jan 17, 2018

View reviewed changes

Mic92 self-assigned this Jan 18, 2018

thefloweringash added 2 commits January 18, 2018 20:42

buildRustCrate: Allow arbitrary attributes in crateOverrides

62dcb3d

cargo-vendor: Build from source using carnix

be797f7

Removes a binary bootstrap, and enables cargo-vendor on aarch64.

Mic92 reviewed Jan 19, 2018

View reviewed changes

jtojnar mentioned this pull request Jan 20, 2018

librsvg: 2.40.18 -> 2.42.0 #34083

Merged

8 tasks

GrahamcOfBorg reviewed Jan 21, 2018

View reviewed changes

Mic92 mentioned this pull request Jan 21, 2018

cargo-vendor-bin: init at 0.1.13 #34082

Closed

8 tasks

GrahamcOfBorg reviewed Jan 21, 2018

View reviewed changes

Mic92 merged commit 8ee5433 into NixOS:master Feb 3, 2018

garbas mentioned this pull request Mar 9, 2018

[WIP] Rust platform improvements #34034

Closed

dezgeg mentioned this pull request May 20, 2018

Build cargo-vendor from source #32254

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cargo-vendor: Build from source using carnix #33980

cargo-vendor: Build from source using carnix #33980

thefloweringash commented Jan 17, 2018

Mic92 commented Jan 17, 2018

Mic92 Jan 17, 2018

Mic92 Jan 17, 2018

Mic92 Jan 17, 2018

P-E-Meunier Jan 17, 2018

thefloweringash Jan 18, 2018

Mic92 Jan 18, 2018 •

edited

Loading

P-E-Meunier Jan 17, 2018

Mic92 Jan 17, 2018

Mic92 Jan 17, 2018 •

edited

Loading

P-E-Meunier commented Jan 17, 2018

dezgeg commented Jan 19, 2018

thefloweringash commented Jan 19, 2018 •

edited

Loading

Mic92 Jan 19, 2018

thefloweringash Jan 19, 2018 •

edited

Loading

Mic92 Jan 19, 2018

dezgeg commented Jan 19, 2018

Mic92 Jan 19, 2018

thefloweringash Jan 19, 2018

Mic92 Jan 19, 2018 •

edited

Loading

thefloweringash Jan 19, 2018

Mic92 Jan 19, 2018

Mic92 commented Jan 19, 2018

bachp commented Jan 19, 2018

Mic92 commented Jan 19, 2018 •

edited

Loading

Mic92 commented Jan 21, 2018

GrahamcOfBorg left a comment

GrahamcOfBorg left a comment

Mic92 commented Jan 21, 2018

grahamc commented Jan 21, 2018

GrahamcOfBorg left a comment

GrahamcOfBorg left a comment

GrahamcOfBorg left a comment

GrahamcOfBorg left a comment

cargo-vendor: Build from source using carnix #33980

cargo-vendor: Build from source using carnix #33980

Conversation

thefloweringash commented Jan 17, 2018

Motivation for this change

Things done

Mic92 commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

P-E-Meunier commented Jan 17, 2018

dezgeg commented Jan 19, 2018

thefloweringash commented Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

thefloweringash Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dezgeg commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 commented Jan 19, 2018

bachp commented Jan 19, 2018

Mic92 commented Jan 19, 2018 • edited Loading

Mic92 commented Jan 21, 2018

GrahamcOfBorg left a comment

Choose a reason for hiding this comment

GrahamcOfBorg left a comment

Choose a reason for hiding this comment

Mic92 commented Jan 21, 2018

grahamc commented Jan 21, 2018

GrahamcOfBorg left a comment

Choose a reason for hiding this comment

GrahamcOfBorg left a comment

Choose a reason for hiding this comment

GrahamcOfBorg left a comment

Choose a reason for hiding this comment

GrahamcOfBorg left a comment

Choose a reason for hiding this comment

Mic92 Jan 18, 2018 •

edited

Loading

Mic92 Jan 17, 2018 •

edited

Loading

thefloweringash commented Jan 19, 2018 •

edited

Loading

thefloweringash Jan 19, 2018 •

edited

Loading

Mic92 Jan 19, 2018 •

edited

Loading

Mic92 commented Jan 19, 2018 •

edited

Loading