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.

Sign up for GitHub

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

Jump to bottom

buildRustCrate: add support for renaming crates #68296

Merged
merged 1 commit into from Sep 10, 2019
Merged

buildRustCrate: add support for renaming crates #68296

merged 1 commit into from Sep 10, 2019

Conversation

danieldk
Copy link
Contributor

@danieldk danieldk commented Sep 8, 2019 •
edited

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 @

@danieldk danieldk mentioned this pull request Sep 8, 2019
Closed
@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.

andir
andir reviewed Sep 8, 2019
View reviewed changes
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 👍.

gilligan
gilligan suggested changes Sep 8, 2019
View reviewed changes
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.

@gilligan gilligan mentioned this pull request Sep 8, 2019
Closed
@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 •
edited

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

@danieldk danieldk mentioned this pull request Sep 8, 2019
Merged
@danieldk
buildRustCrate: add support for renaming crates
85c6d72 
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!

@andir andir merged commit a69a6c1 into NixOS:master Sep 10, 2019
This was referenced Sep 10, 2019
Merged
Merged
@danieldk danieldk mentioned this pull request Sep 17, 2019
Closed
@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 oxalica mentioned this pull request Oct 2, 2019
Closed
@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
Reviewers

@gilligan gilligan gilligan approved these changes

@andir andir andir approved these changes

Assignees
No one assigned
Labels
6.topic: rust 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@danieldk @andir @gilligan @oxalica