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

Support renamed crates #22

Closed
kolloch opened this issue Sep 2, 2019 · 9 comments · Fixed by #24
Closed

Support renamed crates #22

kolloch opened this issue Sep 2, 2019 · 9 comments · Fixed by #24
Assignees
Labels
before-1.0 This needs to be worked on/fixed before a 1.0. enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@kolloch
Copy link
Collaborator

kolloch commented Sep 2, 2019

The information about package renames is exposed by cargo metadata and should therefore be easy to use. Unfortunately, buildRustCrate has no straight-forward support for this.

User docs for renamed crates.

Help wanted: If someone could make it work by hand-coding a minimalistic nix build file, that would be much appreciated. I think that we can hack our way to success even with the current buildRustCrate but explicit support for crate renames in buildRustCrate would be better. Both is fine for now.

@kolloch kolloch added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Sep 2, 2019
@kolloch kolloch added the before-1.0 This needs to be worked on/fixed before a 1.0. label Sep 2, 2019
@gilligan
Copy link

gilligan commented Sep 2, 2019

I'd like to try and provide something but i'm trying to first understand correctly what is going on in this concrete case:

  • rand metadata:
    {
      "name": "rand",
      "version": "0.7.0",
      "id": "rand 0.7.0 (path+file:///home/gilligan/dev/github/rust-random/rand)",
      "license": "MIT OR Apache-2.0",
      "license_file": null,
      "description": "Random number generators and other randomness functionality.\n",
      "source": null,
      "dependencies": [
        {
          "name": "getrandom",
          "source": "registry+https://github.com/rust-lang/crates.io-index",
          "req": "^0.1.1",
          "kind": null,
          "rename": "getrandom_package",
          "optional": true,
          "uses_default_features": true,
          "features": [],
          "target": null
        },

Looking at the output of cargo metadata i can see that getrandom has been renamed to getrandom_package.

  • rand Cargo.toml

Looking at the Cargo.toml of rand:

# Do not depend on 'getrandom_package' directly; use the 'getrandom' feature!
# This is a dependency because: we forward wasm feature flags
# This is renamed because: we need getrandom to depend on rand_core/getrandom

TL;DR: rand used to have a dependency called getrandom, this was renamed to getrandom_package and instead of depending on this directly, the getrandom feature should be specified.


EDIT: So actually, while a carnix based build clearly trips on rand what I am seeing is a failed build because of flate2:

Cargo invoked rustc:

--cfg 'feature="miniz_oxide"'
--cfg 'feature="rust_backend"'

--extern crc32fast
--extern libc
--extern miniz_oxide

Carnix invoked rustc:

--cfg feature="miniz_oxide"
--cfg feature="rust_backend"

--extern crc32fast
--extern libc

Build Error:

error[E0463]: can't find crate for `miniz_oxide`
   --> src/ffi.rs:469:5
    |
469 |     extern crate miniz_oxide;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^ can't find crate

error: aborting due to previous error

So miniz_oxide is specified as feature but not as dependency in the rustc command which seems to cause the error.

EDIT 2:

@kolloch I think the problem that i was seeing with flate2 is something completely independent? So If you try a nix-build -A hydra-cli on the repo/branch i mentioned in #19 building flate2 fails but it works after patching Cargo.nix as follows:

diff --git a/Cargo.nix b/Cargo.nix
index b47b41f..68f72d0 100644
--- a/Cargo.nix
+++ b/Cargo.nix
@@ -1009,10 +1009,7 @@ rec {
         dependencies = {
           "crc32fast" = "crc32fast 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)";
           "libc" = "libc 0.2.62 (registry+https://github.com/rust-lang/crates.io-index)";
-          "miniz_oxide" = {
-            packageId = "miniz_oxide 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)";
-            target = ((target."arch" == "wasm32") && (!(target."os" == "emscripten")));
-          };
+          "miniz_oxide" = "miniz_oxide 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)";
         };
         features = {
           "default" = [ "miniz-sys" ];

Without the patch miniz_oxide is added as feature but not added via --extern and the build fails. Should I turn this into a separate ticket?

@gilligan
Copy link

gilligan commented Sep 3, 2019

Backing off a bit from the concrete problem I am looking at i'd like to ask:

buildRustCrate
You say you would like for this to be handled by buildRustCrate but I don't see how this would be possible. It seems to me like buildRustCrate would need to know about renaming to begin with. So this is something that f.ex the nix code generated by crate2nix would have to pass in.

renaming semantics
Also, i'm wondering if you perhaps have a clear idea about how the semantics behind the renaming because this isn't quite clear to me. Taking the example from rand:

[dependencies]
getrandom_package = { version = "0.1.1", package = "getrandom", optional = true }

What logic do you reckon this should lead to? I guess:

Fetch getrandom package and make it available as getrandom_package and supply this via --extern getrandom_package=<path to getrandom > ?

This could be done in crate2nix without touching buildRustCrate I think?

@gilligan
Copy link

gilligan commented Sep 4, 2019

I created https://github.com/gilligan/rs-nix-test just to have a minimal reproducible test case. I'll start looking at both the code of buildRustCrate and the nix code created by crate2nix tomorrow.

@danieldk
Copy link
Contributor

danieldk commented Sep 7, 2019

@gilligan thanks for making the minimal example. It turns out to be fairly simple to change the generated Nix to support the replacement. The basic idea:

  1. Create a copy of getrandom with a different attribute name (package ID). In this copy, add libName and set it to the renamed name of the package (we can't change crateName, because the crate cannot be retrieved if we do).
  2. For rand, add the renamed package as an optional dependency. Use the package ID from step (1).

The diff against crate2nix.nix from @gilligan :

https://gist.github.com/danieldk/a3c2a091134fd75db079a37c3c486604

The build now fails for another reason, but at least rustc is called with the right --extern options.

@danieldk
Copy link
Contributor

danieldk commented Sep 7, 2019

Ok, I think this is a dead end. The error is:

error[E0277]: `?` couldn't convert the error to `rand_core::error::Error`
  --> src/rngs/os.rs:78:24
   |
78 |         getrandom(dest)?;
   |                        ^ the trait `std::convert::From<getrandom_package::error::Error>` is not implemented for `rand_core::error::Error`
   |
   = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
   = help: the following implementations were found:
             <rand_core::error::Error as std::convert::From<getrandom::error::Error>>
             <rand_core::error::Error as std::convert::From<std::num::NonZeroU32>>
   = note: required by `std::convert::From::from`

error: aborting due to previous error

I think the problem is that rand is compiled with:

--extern getrandom_package=/nix/store/7wnvc0j1bi64n06sfd6pks377w4gvxnq-rust_getrandom-0.1.11/lib/libgetrandom_package-b39e9273b9.rlib

and rand_core with:

--extern getrandom=/nix/store/068xaxfawj1vmnrl6nxd3abcmc5r0wrm-rust_getrandom-0.1.11/lib/libgetrandom-b39e9273b9.rlib

So, the From implementation from rand_core only applies to the Error type from getrandom and not getrandom_package. So, we really need the following --extern flag for rand:

--extern getrandom_package=/nix/store/068xaxfawj1vmnrl6nxd3abcmc5r0wrm-rust_getrandom-0.1.11/lib/libgetrandom-b39e9273b9.rlib

However, buildRustCrate always uses a flag of the form name=[...]libname[...]. So I believe that renamed crates cannot be supported without a change in buildRustCrate.

@gilligan
Copy link

gilligan commented Sep 7, 2019

@danieldk yes I think so as well. My thought the other day was to have crate2Nix pass a “renames” argument to buildRustCrate but i haven’t had the time to look into this again since.

@danieldk
Copy link
Contributor

danieldk commented Sep 8, 2019

I got it working. Patch against buildRustCrate and your sample project:

https://gist.github.com/danieldk/9d78945828dc5d0bcebc0e0b913d5211

I still have to clean it up a little. renames now also contains identity mappings, and I probably should rename it to crateRenames. Finally, I guess renames should also be extracted from buildDependencies.

@danieldk
Copy link
Contributor

danieldk commented Sep 8, 2019

I made a draft PR with the buildRustCrate changes:

NixOS/nixpkgs#68296

The updated changes in the generated crate2nix:

https://gist.github.com/danieldk/2984e65c71e7c0867e528a02194562aa

@kolloch
Copy link
Collaborator Author

kolloch commented Sep 8, 2019

Awesome! Let's wait until support lands in nixos. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
before-1.0 This needs to be worked on/fixed before a 1.0. enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants