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

Carnix: 0.8 -> 0.9 #50923

Closed
wants to merge 2 commits into from
Closed

Carnix: 0.8 -> 0.9 #50923

wants to merge 2 commits into from

Conversation

P-E-Meunier
Copy link
Contributor

Motivation for this change

Carnix 0.9 gives a way to merge the crates-io.nix from different packages into a single package, in two different ways:

  • By producing a list of packages, and a way to recover crates-io.nix from that list, using carnix generate-crates-io crates-io.list.
  • By introducing a new command, carnix merge, to merge the contents of multiple files (given on the standard input) into a single file.

It would be very cool to have just a crates-io.list, and generate crates-io.nix as a derivation. However:

  • Carnix (required to produce crates-io.nix) still needs a crates-io.nix to be compiled.
  • carnix generate-crates-io needs to download all the packages in the list. Can we let Hydra do that?
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@dywedir
Copy link
Member

dywedir commented Nov 27, 2018

@GrahamcOfBorg build carnix cargo-vendor

@GrahamcOfBorg
Copy link

Success on x86_64-darwin (full log)

Attempted: carnix, cargo-vendor

Partial log (click to expand)


warning: redundant linker flag specified for library `z`

installing
post-installation fixup
strip is /nix/store/mvpvjar6m4jpjcz48715w2pax53djv6g-cctools-binutils-darwin/bin/strip
stripping (with command strip and flags -S) in /nix/store/vapjx5gqffkkynl0kf3xml10d9qq9nfg-rust_cargo-vendor-0.1.13/lib  /nix/store/vapjx5gqffkkynl0kf3xml10d9qq9nfg-rust_cargo-vendor-0.1.13/bin
patching script interpreter paths in /nix/store/vapjx5gqffkkynl0kf3xml10d9qq9nfg-rust_cargo-vendor-0.1.13
/nix/store/ic0r1dvbva8ljja0frdn982m35dm6vs0-rust_carnix-0.9.1
/nix/store/vapjx5gqffkkynl0kf3xml10d9qq9nfg-rust_cargo-vendor-0.1.13

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: carnix, cargo-vendor

Partial log (click to expand)

post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/s6f8zm5r8vr01024sgmkxpgpjg4bcn37-rust_carnix-0.9.1
shrinking /nix/store/s6f8zm5r8vr01024sgmkxpgpjg4bcn37-rust_carnix-0.9.1/bin/cargo-generate-nixfile
shrinking /nix/store/s6f8zm5r8vr01024sgmkxpgpjg4bcn37-rust_carnix-0.9.1/bin/carnix
strip is /nix/store/n4hb93w6j076xcjw5pm09rdmc09s075b-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/s6f8zm5r8vr01024sgmkxpgpjg4bcn37-rust_carnix-0.9.1/lib  /nix/store/s6f8zm5r8vr01024sgmkxpgpjg4bcn37-rust_carnix-0.9.1/bin
patching script interpreter paths in /nix/store/s6f8zm5r8vr01024sgmkxpgpjg4bcn37-rust_carnix-0.9.1
checking for references to /build in /nix/store/s6f8zm5r8vr01024sgmkxpgpjg4bcn37-rust_carnix-0.9.1...
/nix/store/s6f8zm5r8vr01024sgmkxpgpjg4bcn37-rust_carnix-0.9.1
/nix/store/6lhchnbgaadlh7hqq6sw7dlri6h2kxgs-rust_cargo-vendor-0.1.13

@dywedir dywedir requested a review from Mic92 November 27, 2018 16:22
@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: carnix, cargo-vendor

Partial log (click to expand)

installing
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/ai26m6j8zhvl99x734kccsplcf927lfn-rust_cargo-vendor-0.1.13
shrinking /nix/store/ai26m6j8zhvl99x734kccsplcf927lfn-rust_cargo-vendor-0.1.13/bin/cargo-vendor
strip is /nix/store/qjrnv0qw44bw1hc23zhfh26xd1c25dfs-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/ai26m6j8zhvl99x734kccsplcf927lfn-rust_cargo-vendor-0.1.13/lib  /nix/store/ai26m6j8zhvl99x734kccsplcf927lfn-rust_cargo-vendor-0.1.13/bin
patching script interpreter paths in /nix/store/ai26m6j8zhvl99x734kccsplcf927lfn-rust_cargo-vendor-0.1.13
checking for references to /build in /nix/store/ai26m6j8zhvl99x734kccsplcf927lfn-rust_cargo-vendor-0.1.13...
/nix/store/bhy3c4csc9nssb3yvm5q910lhrg6711a-rust_carnix-0.9.1
/nix/store/ai26m6j8zhvl99x734kccsplcf927lfn-rust_cargo-vendor-0.1.13

@Mic92
Copy link
Member

Mic92 commented Nov 27, 2018

This addition sounds great!
it seems like the old cache format is no longer valid:

$ carnix generate-crates-io crates-io.list
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error(Io(Custom { kind: InvalidData, error: StringError("stream did not contain valid UTF-8") }), State { next_error: N
one, backtrace: InternalBacktrace { backtrace: None } })', libcore/result.rs:1009:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.
$ rm /home/joerg/.cargo/nix-cache
$ carnix generate-crates-io crates-io.list
Prefetching aho-corasick-0.6.8
Prefetching ansi_term-0.11.0
Prefetching argon2rs-0.2.5
Prefetching arrayvec-0.4.7

I have not understood yet how the merge command is supposed to work.
What is in the first argument that the program takes?
Also how would the workflow for contributors look like?
Is the the idea that contributors first generate their crates-io.nix file as usual
and then in a later commit we merge the content into the the single crates-io.nix file and recreate the now smaller per-package crrates-io.nix files?
Finally it would be nice to also have the steps documented in our rust nixpkgs documentation.

cc @andir @Ekleog

@P-E-Meunier
Copy link
Contributor Author

The cache has changed, it no longer needs sqlite and uses plain text instead. Carnix 0.9.1 has a bug in that part actually, which was fixed by Carnix 0.9.2. I'd prefer to keep working on compiling Rust to WASM using this before submitting a new PR.

carnix merge takes several versions of crates-io.nix as input, and outputs a merger of them. These files need to have been produced with carnix >= 0.9, as the parsing is extremely basic and relies on markers placed in the file.

The idea is that contributors submit the larger single crates-io.nix in their PRs. When Carnix reaches 1.0, we will need to check that these commits only add to that file. There is not "per crate" crates-io.nix, only a Cargo.nix.

I wonder what the Hydra will think of that. Will it speed up release mode deployments by pre-compiling common libraries? That would be very cool and would make Nix the most convenient platform for writing web services in Rust.

@Mic92
Copy link
Member

Mic92 commented Nov 27, 2018

Could you maybe add an better error message if somebody is using the sqlite file or change the name of the cache file? I had to use strace to find out why carnix failed to update for me, other people might not find out the reason for that.

Can you make an concrete example how I have to run carnix merge? It does not accept multiple arguments:

[nix-shell:~/git/nixpkgs/pkgs/build-support/rust]$ cp crates-io.nix crates-io2.nix
[nix-shell:~/git/nixpkgs/pkgs/build-support/rust]$ carnix merge crates-io.nix crates-io2.nix
error: Found argument 'crates-io2.nix' which wasn't expected, or isn't valid in this context

USAGE:
    carnix merge [file]

For more information try --help
$ cat crates-io.nix | carnix merge
{ lib, buildRustCrate, buildRustCrateHelpers }:
with buildRustCrateHelpers;
let inherit (lib.lists) fold;
    inherit (lib.attrsets) recursiveUpdate;
in
rec {
}
$ carnix merge crates-io.nix
{ lib, buildRustCrate, buildRustCrateHelpers }:
with buildRustCrateHelpers;
let inherit (lib.lists) fold;
    inherit (lib.attrsets) recursiveUpdate;
in
rec {
}

@Mic92 Mic92 mentioned this pull request Dec 10, 2018
9 tasks
Copy link
Member

@ryantm ryantm left a comment

Choose a reason for hiding this comment

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

Please resolve merge conflicts and address mic92s questions

@P-E-Meunier
Copy link
Contributor Author

I just pushed #56314, which addresses @Mic92 's questions in the documentation. The error messages are not much better now, but at least the documentation is clear about them.

@ghost
Copy link

ghost commented Feb 25, 2019

Hi @P-E-Meunier
I didn't see any docs addition in #56314

@P-E-Meunier
Copy link
Contributor Author

Sorry, I probably forgot to git add it. I have updates to buildRustCrate to push anyway, let me open a new PR.

@ryantm
Copy link
Member

ryantm commented Feb 25, 2019

Oops, sorry. I thought "documentation" was just referring to the command line error messages.

@P-E-Meunier
Copy link
Contributor Author

Oops, I had completely rewritten the section about Carnix, but it seems I forgot to commit and it got wiped by a rebase. I will probably write it again one of these days, because the documentation is severely outdated, but I right now I just feel "git'ed".

@ghost
Copy link

ghost commented Feb 25, 2019

😢

Thank you for your work!

Could you please also explain: what should the default.nix file look like to work with Cargo.nix and contain some meta?

Something like this?

{ stdenv, defaultCrateOverrides, fetchFromGitHub }:

((import ./Cargo.nix).pijul {}).override {
  crateOverrides = defaultCrateOverrides // {
    pijul = attrs: rec {
      name = "pijul-${version}";
      version = "0.11.0";

      meta = with stdenv.lib; {
        description = "A distributed version control system";
        homepage = https://pijul.org;
        license = with licenses; [ gpl2Plus ];
        maintainers = [ maintainers.gal_bolle ];
        platforms = platforms.all;
      };
    };
  };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants