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

buildRustCrate: add support for renaming crates #68296

Merged
merged 1 commit into from
Sep 10, 2019

Conversation

danieldk
Copy link
Contributor

@danieldk danieldk commented Sep 8, 2019

Motivation for this change

Before this change, buildRustCrate always called rustc with

--extern libName=[...]libName[...]

However, Cargo permits using a different name under which a dependency
is known to a crate. For example, rand 0.7.0 uses:

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

Which introduces the getrandom dependency such that it is known as
getrandom_package to the rand crate. In this case, the correct extern
flag is of the form

--extern getrandom_package=[...]getrandom[...]

which is currently not supported. In order to support such cases, this
change introduces a crateRenames argument to buildRustCrate. This
argument is an attribute set of dependencies that should be
renamed. In this case, crateRenames would be:

{
  "getrandom" = "getrandom_package";
}

The extern options are then built such that if the libName occurs as
an attribute in this set, it value will be used as the local
name. Otherwise libName will be used as before.

This change should not affect existing derivations that use buildRustCrate --
an empty attrset is used for crateRenames when this argument is not provided,
applying the old behavior.

Please review very carefully, I have never looked at the buildRustCrate
code before ;).

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

ping @P-E-Meunier @kolloch

cc @

@ofborg ofborg bot added 6.topic: rust 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Sep 8, 2019
@danieldk danieldk marked this pull request as ready for review September 8, 2019 07:00
@Mic92 Mic92 requested a review from andir September 8, 2019 08:30
@andir andir requested a review from gilligan September 8, 2019 10:37
@andir
Copy link
Member

andir commented Sep 8, 2019

Added @gilligan as I had a conversation about this feature with him during the week.

Copy link
Member

@andir andir left a comment

Choose a reason for hiding this comment

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

After a first glance it looks okay. Could you add a testcase to the buildRustCrateTests attribute?

The tests currently pass and are even cached meaning you are really not causing any changes to "old" expressions 👍.

Copy link
Contributor

@gilligan gilligan left a comment

Choose a reason for hiding this comment

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

The changes per se look good to me! It's in line with what I was also mentioning on the crates2nix repo. Thank you so much for taking the time and initiative to fix it :)

I agree with @andir that there should be some tests with this change. Maybe you can just use the same minimal example that I put in https://github.com/gilligan/rs-nix-test to verify that things are working.

With tests added i'd be happy to see this merged. Oh, and I think we should also get this into 18.09, right?

@danieldk
Copy link
Contributor Author

danieldk commented Sep 8, 2019

Thanks for the comments! I have added a test case.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Sep 8, 2019
@danieldk danieldk changed the title buildRustPackage: add support for renaming crates buildRustCrate: add support for renaming crates Sep 8, 2019
@danieldk
Copy link
Contributor Author

danieldk commented Sep 8, 2019

The last two pushes were just replacing occurrences of buildRustPackage to buildRustCrate. Apparently, I was sleeping ;).

Before this change, buildRustCrate always called rustc with

--extern libName=[...]libName[...]

However, Cargo permits using a different name under which a dependency
is known to a crate. For example, rand 0.7.0 uses:

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

Which introduces the getrandom dependency such that it is known as
getrandom_package to the rand crate. In this case, the correct extern
flag is of the form

--extern getrandom_package=[...]getrandom[...]

which is currently not supported. In order to support such cases, this
change introduces a crateRenames argument to buildRustCrate. This
argument is an attribute set of dependencies that should be renamed. In
this case, crateRenames would be:

{
  "getrandom" = "getrandom_package";
}

The extern options are then built such that if the libName occurs as
an attribute in this set, it value will be used as the local
name. Otherwise libName will be used as before.
@danieldk
Copy link
Contributor Author

danieldk commented Sep 8, 2019

Found and fixed a bug while preparing a PR for crate2nix to support renames: the lookup should be by crateName and not libName, otherwise this breaks when crateName != libName. I have also added a test for this case.

@gilligan
Copy link
Contributor

gilligan commented Sep 9, 2019

Awesome work!

@oxalica
Copy link
Contributor

oxalica commented Oct 1, 2019

I found it do renaming only over crate names, without considering crate version.

This seems to cause problem when a crate depends on multiple versions of the same named crate.
For example:

a = { version = "0.1", package = "libc" }
b = { version = "0.2", package = "libc" }

We cannot even provide a proper crateRenames in this case.

@danieldk

@danieldk
Copy link
Contributor Author

danieldk commented Oct 2, 2019

For example:

a = { version = "0.1", package = "libc" }
b = { version = "0.2", package = "libc" }

We cannot even provide a proper crateRenames in this case.

Good find!

rustc extern flags:

--extern a=/home/daniel/rename-test/target/debug/deps/liblibc-da86144831204d21.rlib --extern b=/home/daniel/rename-test/target/debug/deps/liblibc-a4b0875e7e16a993.rlib

So, I guess crateRenames needs to be keyed on the version besides the crate name. Another problem is that upstream tools may not even create valid Nix. E.g. crate2nix (@kolloch):

        dependencies = {
          "libc" = {
            packageId = "libc 0.1.12 (registry+https://github.com/rust-lang/crates.io-index)";
            rename = "b";
          };
          "libc" = {
            packageId = "libc 0.2.62 (registry+https://github.com/rust-lang/crates.io-index)";
            rename = "b";
          };
        };

@oxalica
Copy link
Contributor

oxalica commented Oct 2, 2019

@danieldk I've opened a new issue #70270 for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: rust 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants