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.7.2 -> 0.8.10 #40587

Merged
merged 5 commits into from
Oct 27, 2018
Merged

Carnix: 0.7.2 -> 0.8.10 #40587

merged 5 commits into from
Oct 27, 2018

Conversation

P-E-Meunier
Copy link
Contributor

Motivation for this change

This new version of Carnix splits its input into two parts:

  • crates from crates.io, meant to be included in nixpkgs.
  • local crates and a tiny translation of the lockfile (which might get even smaller in the future).
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.

@P-E-Meunier P-E-Meunier mentioned this pull request May 16, 2018
8 tasks
@GrahamcOfBorg GrahamcOfBorg added 6.topic: rust 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels May 16, 2018
@Mic92
Copy link
Member

Mic92 commented May 19, 2018

This change looks pretty good to me. The split makes sense.

First of all how do we plan to maintain our crates-io.nix package set?
Have you tested how large the full set would be?

Here a notes regarding the carnix command:

  • the carnix nix subcommand could become a verb like generate-nix or update-nix or just generate so it is more clear what it does
  • carnix build currently returns:
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error(Io(Os { code: 2, kind: NotFound, message: "No such file or directory" }), State { next_error: None, backtrace: None })', libcore/result.rs:945:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.

if cargo is missing. Not adding cargo to $PATH is fine since it allows to switch between rust from nixpkgs and the rust overlay. However the error message could be more clear what is actually missing.

Is it possible to include help messages on subcommand level?

$ carnix --help
carnix 0.8.5
pmeunier <pe@pijul.org>
Generate a nix derivation set from a cargo registry
USAGE:
    carnix [SUBCOMMAND]
FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information
SUBCOMMANDS:
    build       Build project in current directory
    help        Prints this message or the help of the given subcommand(s)
    generate Generate nix expression
    run          

carnix run is currently doing the same as carnix build?

Finally it would be great when doc/languages-frameworks/rust.section.md would be also updated to incorporate this change.

@P-E-Meunier P-E-Meunier mentioned this pull request May 20, 2018
8 tasks
@GrahamcOfBorg GrahamcOfBorg added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 20, 2018
@gebner
Copy link
Member

gebner commented May 20, 2018

cargo-vendor and anything that depends on it is currently broken on master because of the curl-sys build failure. I think we should merge this PR now since it fixes the build failure, and add documentation and polishing later. @P-E-Meunier there is now a merge conflict.

@Mic92
Copy link
Member

Mic92 commented May 20, 2018

I cherry-picked the build fix into this pull request: https://github.com/NixOS/nixpkgs/pull/40808/files

@P-E-Meunier
Copy link
Contributor Author

First of all how do we plan to maintain our crates-io.nix package set?

I don't think I'm the one who should decide. A reasonable middle ground could be to maintain it manually, with the caveat that the format might change again in the future (the features code is not yet minimal).

Another option could be to add a command to carnix, that generates the file from a list of packages and versions, with the goal of not importing tons of useless packages into nixpkgs.

Yet another way could be to generate the whole thing from crates.io and put it online somewhere. I don't have the resources (mostly in terms of time) to setup a server now that would keep importing from crates.io (or even better, from the list of packages on GitHub), but I can provide guidance to someone who would want to do that.

Have you tested how large the full set would be?

No, but I expect it to be quite large. Many parts of the output are still quite verbose.

Another issue is, I'd like to write tests to check that we don't break any package when we change buildRustCrate, but I'm not sure how to write tests for nixpkgs.

@Mic92
Copy link
Member

Mic92 commented May 20, 2018

How did you generate the current list? If we feed in a list of packages, we have in nixpkgs, it should good enough.

UPDATE I can also take care of the documentation if it is clear to me how cratesIO is generated.

@xeji
Copy link
Contributor

xeji commented May 31, 2018

Looks like there was a temporary merge conflict.
@GrahamcOfBorg eval

@GrahamcOfBorg GrahamcOfBorg removed the 2.status: merge conflict This PR has merge conflicts with the target branch label May 31, 2018
@P-E-Meunier
Copy link
Contributor Author

@Mic92: the current list is generated by Carnix, just as before, except that the output is split into two files now.

@Mic92
Copy link
Member

Mic92 commented Jun 8, 2018

@P-E-Meunier How would one create a common crates-io.nix file for all buildRustCrate packages?

@P-E-Meunier
Copy link
Contributor Author

@Mic92 there are lots of crates with lots of versions on crates.io. Importing them all would yield a fast-changing big file in nixpkgs.

What we could do is to only include the ones that are useful for binaries currently packaged in nixpkgs, which would require (1) some manual work to merge new deps into the file, and (2) automated checks on the nixpkgs repository.

Maybe these two tasks could be included in Carnix, at least (1) seems easy.

@Mic92
Copy link
Member

Mic92 commented Jun 9, 2018

I agree we should not have all crate versions in nixpkgs and I also thought about having a list of packages that is used to generate all packages.
Carnix would then need to create one "lockfile" per package one can import the package from and a big crates-io.nix file that is shared across all packages.
All rust packages that are built that way would then be located into the same directory.

@gavinrogers
Copy link

@P-E-Meunier is this what became of the following?
https://www.reddit.com/r/rust/comments/649h6m/using_nix_nixos_as_a_build_system_for_rust_and/
The link seems to 404, unless I don't know how to use nest properly.

@P-E-Meunier
Copy link
Contributor Author

There isn't much to "know" about the Nest, but it's now at https://nest.pijul.com/pmeunier/carnix

@mstone
Copy link
Contributor

mstone commented Jul 17, 2018

Folks,

Is there any work that remains to be done on this PR before it can be merged? (Or are we still stuck trying to figure out how to handle the crates.io-part of the output)?

Thank you!

Michael

@xeji xeji added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 1, 2018
@jasoncarr0
Copy link
Contributor

jasoncarr0 commented Oct 18, 2018

This and #39317 have been sitting around for quite a while.
Would it be possible to merge this if we fixed the conflicts, even if it may create a few larger derivations for now? Carnix is now up to 0.8.5 so we may want to include that.

There are major bug fixes in both, that are necessary for building other packages (e.g. xi-editor requires the fix for local crates).

@P-E-Meunier
Copy link
Contributor Author

I have a non-conflicting version. Also, Carnix changed a little bit since this pull request, a better option could be that we close this and open a different PR.

@GrahamcOfBorg GrahamcOfBorg removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 21, 2018
@P-E-Meunier
Copy link
Contributor Author

Ok, this PR includes #47976, and is mergeable now.

@GrahamcOfBorg
Copy link

Success on x86_64-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/rhcvc49i3wr3kfk3jk8wc3ddp4ypzxy7-rust_cargo-vendor-0.1.13
shrinking /nix/store/rhcvc49i3wr3kfk3jk8wc3ddp4ypzxy7-rust_cargo-vendor-0.1.13/bin/cargo-vendor
strip is /nix/store/vcc4svb8gy29g4pam2zja6llkbcwsyiq-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/rhcvc49i3wr3kfk3jk8wc3ddp4ypzxy7-rust_cargo-vendor-0.1.13/lib  /nix/store/rhcvc49i3wr3kfk3jk8wc3ddp4ypzxy7-rust_cargo-vendor-0.1.13/bin
patching script interpreter paths in /nix/store/rhcvc49i3wr3kfk3jk8wc3ddp4ypzxy7-rust_cargo-vendor-0.1.13
checking for references to /build in /nix/store/rhcvc49i3wr3kfk3jk8wc3ddp4ypzxy7-rust_cargo-vendor-0.1.13...
/nix/store/cax28cra2hr7f1v4q792hrjmdhv599v2-rust_carnix-0.8.9
/nix/store/rhcvc49i3wr3kfk3jk8wc3ddp4ypzxy7-rust_cargo-vendor-0.1.13

@P-E-Meunier P-E-Meunier changed the title Carnix: 0.7.2 -> 0.8.9 Carnix: 0.7.2 -> 0.8.10 Oct 24, 2018
@P-E-Meunier
Copy link
Contributor Author

I just fixed @sjmackenzie's issue, his example compiles with Carnix 0.8.10. Feel free to report other bugs.

@jasoncarr0
Copy link
Contributor

jasoncarr0 commented Oct 25, 2018

Well, if you're planning on fixing them now then https://github.com/xi-editor/xi-editor was one that previously gave me trouble (since some crates are only local, and not uploaded).

EDIT: https://nest.pijul.com/pmeunier/carnix/discussions/15#46dd7fb6-3a19-4a51-9f0a-382a47fbab48

@sjmackenzie
Copy link
Contributor

Pierre, thank you so much, I greatly appreciate that! 🍻

@sjmackenzie
Copy link
Contributor

Ah wait, @P-E-Meunier just tried to take it for a spin but the patch doesn't pass muster.

error: attribute '0.8.10' missing, at /home/stewart/workspace/nixpkgs/pkgs/build-support/rust/carnix.nix:9:12

@sjmackenzie
Copy link
Contributor

sjmackenzie commented Oct 25, 2018

The carnix 0.8.10 executable is generating code without panicking, but the generated code seems to be incorrect.

[nix-shell:~/dev/fractalide/rust-cardano]$ carnix build
    Updating registry `https://github.com/rust-lang/crates.io-index`
error: undefined variable 'buildRustCrateHelpers' at /home/stewart/dev/fractalide/rust-cardano/Cargo.nix:3:6
$ carnix generate-nix
Prefetching cardano-0.1.0
nix-prefetch-url returned:
error: unable to download 'https://crates.io/api/v1/crates/cardano/0.1.0/download': HTTP error 404

EDIT: after reading a comment in this thread: https://nest.pijul.com/pmeunier/carnix/discussions/15#46dd7fb6-3a19-4a51-9f0a-382a47fbab48
the correct way to generate Cargo.nix is by running carnix generate-nix --src ./.
and I was forgetting to pass in the -I nixpkgs=/path/to/nixpkgs/that/contains/carnix-0.8.10

At least carnix run -I nixpkgs=/path/to/nixpkgs/that/contains/carnix-0.8.10 works well!

@sjmackenzie
Copy link
Contributor

sjmackenzie commented Oct 25, 2018

Okay I've found a last little bug in the software (at least on this execution path :-) )

To put it briefly:
when Cargo.toml has crate-type = ["staticlib", "cdylib", "dylib" ] shared objects are not generated : doesn't work
when Cargo.toml has crate-type = ["cdylib", "dylib" ] shared objects are generated : works

For detailed steps to reproduce please go here: https://nest.pijul.com/pmeunier/carnix/discussions/16

@sjmackenzie
Copy link
Contributor

@P-E-Meunier nearly! :-)

$ nix-shell -p carnix cargo -I nixpkgs=/home/stewart/dev/fractalide/nixpkgs
error: value is a string while a list was expected, at /home/stewart/dev/fractalide/nixpkgs/pkgs/build-support/rust/build-rust-crate/build-crate.nix:47:39

@P-E-Meunier
Copy link
Contributor Author

Yes, you should regenerate your Cargo.nix, the types have changed.

@sjmackenzie
Copy link
Contributor

sjmackenzie commented Oct 26, 2018

Na mate, I haven't got to the stage to regenerate my Cargo.nix file. It's failing when loading carnix-0.8.11 into the nix-shell. In other words there seems to be a syntax in your most recent patch. Here's the error message https://gist.github.com/GrahamcOfBorg/712adeff95a6d8448d83780b513aa89e

@P-E-Meunier
Copy link
Contributor Author

Ok, fixed.

@sjmackenzie
Copy link
Contributor

Ah, the error persists: given the most recent patch only changes a comment and code.
(e6e9bb4)

$ nix-shell -p carnix cargo -I nixpkgs=/home/stewart/dev/fractalide/nixpkgs
error: value is a string while a list was expected, at /home/stewart/dev/fractalide/nixpkgs/pkgs/build-support/rust/build-rust-crate/build-crate.nix:47:39

@sjmackenzie
Copy link
Contributor

How very odd, I'm on 08d8ff0 and I'm getting can't find crate errors:

$ nix-shell -p carnix cargo -I nixpkgs=/home/stewart/dev/fractalide/nixpkgs
...
error: extern location for linked_hash_map does not exist: /nix/store/acpkp8m08szmc018a1x4k5d77rz3pr8y-rust_linked-hash-map-0.4.2/lib/liblinked_hash_map-30c6cccf41.so

error[E0463]: can't find crate for `linked_hash_map`
  --> src/lib.rs:40:1
   |
40 | extern crate linked_hash_map;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't find crate

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0463`.
error: extern location for either does not exist: /nix/store/dimshwfs3cdbv45p6dgyfp8v0yrkd85b-rust_either-1.5.0/lib/libeither-6ff867d1be.so

error[E0463]: can't find crate for `either`
  --> src/lib.rs:27:1
   |
27 | extern crate either;
   | ^^^^^^^^^^^^^^^^^^^^ can't find crate

error: aborting due to 2 previous errors

@P-E-Meunier
Copy link
Contributor Author

Thanks @sjmackenzie, and sorry about the multiple failed attempts. I believe it's working now.

@dywedir
Copy link
Member

dywedir commented Oct 26, 2018

@GrahamcOfBorg build carnix cargo-vendor

@sjmackenzie
Copy link
Contributor

LGTM! @P-E-Meunier it's always a pleasure using your software mate. Thanks!

@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/82c71cxwn28krilvmimp9h586ssm9s1l-rust_cargo-vendor-0.1.13
shrinking /nix/store/82c71cxwn28krilvmimp9h586ssm9s1l-rust_cargo-vendor-0.1.13/bin/cargo-vendor
strip is /nix/store/p9akxn2sfy4wkhqdqa3li97pc6jaz3r1-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/82c71cxwn28krilvmimp9h586ssm9s1l-rust_cargo-vendor-0.1.13/lib  /nix/store/82c71cxwn28krilvmimp9h586ssm9s1l-rust_cargo-vendor-0.1.13/bin
patching script interpreter paths in /nix/store/82c71cxwn28krilvmimp9h586ssm9s1l-rust_cargo-vendor-0.1.13
checking for references to /build in /nix/store/82c71cxwn28krilvmimp9h586ssm9s1l-rust_cargo-vendor-0.1.13...
/nix/store/8gldm5yzpjwy2hq0n4d72y52jc5sqj21-rust_carnix-0.8.11
/nix/store/82c71cxwn28krilvmimp9h586ssm9s1l-rust_cargo-vendor-0.1.13

@sjmackenzie
Copy link
Contributor

@qknight @gebner seems like this is ready to be merged!

@GrahamcOfBorg
Copy link

Timed out, unknown build status on x86_64-linux (full log)

Attempted: carnix, cargo-vendor

Partial log (click to expand)

cannot build derivation '/nix/store/sn4cdhv6fgycqi8qxxjkk6nns57zw5nr-rust_serde_derive-1.0.18.drv': 6 dependencies couldn't be built
cannot build derivation '/nix/store/23grkvr0ky5nczjsq5ih5dqq08g4lvg4-rust_serde_derive-1.0.80.drv': 5 dependencies couldn't be built
cannot build derivation '/nix/store/8d4bv336n025b4np5y7p2mi8q4dskmb8-rust_carnix-0.8.11.drv': 51 dependencies couldn't be built
cannot build derivation '/nix/store/wh1msg1mkqxwc2vx904hm2mil01zm35c-rust_crates-io-0.11.0.drv': 30 dependencies couldn't be built
cannot build derivation '/nix/store/53fv3yf5hfv20m5h6klwyhirv5vq296k-rust_docopt-0.8.1.drv': 19 dependencies couldn't be built
cannot build derivation '/nix/store/4lqk6gj31ixz1j7vzsq2rigj3jij6vhf-rust_git2-curl-0.7.0.drv': 20 dependencies couldn't be built
cannot build derivation '/nix/store/cz8f3qdqpv49idr5rc0lpax23lmjx8r5-rust_ignore-0.2.2.drv': 18 dependencies couldn't be built
cannot build derivation '/nix/store/mqyir1kk5y6wa8f5ac2sljwcj3m966js-rust_cargo-0.22.0.drv': 78 dependencies couldn't be built
cannot build derivation '/nix/store/zfhr61dwj46b7q1b58d0bs99yfn69agq-rust_cargo-vendor-0.1.13.drv': 79 dependencies couldn't be built
error: build of '/nix/store/8d4bv336n025b4np5y7p2mi8q4dskmb8-rust_carnix-0.8.11.drv', '/nix/store/zfhr61dwj46b7q1b58d0bs99yfn69agq-rust_cargo-vendor-0.1.13.drv' failed

@qknight qknight merged commit ae3b465 into NixOS:master Oct 27, 2018
@P-E-Meunier
Copy link
Contributor Author

P-E-Meunier commented Nov 13, 2018

I don't know why buildRustCrateHelpers was removed from the repository, who did it, nor why I wasn't asked to review that change or to update Carnix to take it into account, but I just spent two hours debugging a project that required it, and kept giving me obscure errors.

The code produced by the version of Carnix that's in nixpkgs requires that line, and that's actually why it was there.

Maybe a bad git merge is responsible for this situation, since I couldn't find any other information about the change. In any case, we need that line (and that file) back.

The last thing I want is to fight with anyone, because I love everybody, and I know everyone spent lots of time on Nix and Nixpkgs, which is a fantastic project. But I'm a little bit upset, since I invested a substantial amount of time into Carnix to make it work out of the box, and accommodated many feature requests from the community.

@P-E-Meunier
Copy link
Contributor Author

Wait, was this entire PR reverted? Why?

@tilpner
Copy link
Member

tilpner commented Nov 13, 2018

Seems to be still present?

@P-E-Meunier
Copy link
Contributor Author

Ok, this is weird. I didn't check it out locally, and only looked at GitHub. I can definitely see it there now, I swear it didn't show up when I wrote that. Thanks.

@sjmackenzie
Copy link
Contributor

sjmackenzie commented Nov 13, 2018

@P-E-Meunier it's not just you, i've seen similar oddities when using github before; search functionality, Mary Celeste code. Not often, but enough to question my sanity.

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.