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

Add support for dependency renaming #24

Merged
merged 1 commit into from
Sep 15, 2019
Merged

Add support for dependency renaming #24

merged 1 commit into from
Sep 15, 2019

Conversation

danieldk
Copy link
Contributor

@danieldk danieldk commented Sep 8, 2019

This PR adds support for renamed dependencies. I have marked this as a draft PR, since it only works iff

NixOS/nixpkgs#68296

gets merged. I will also update the PR later to add a test. This fixes #22.

@danieldk
Copy link
Contributor Author

danieldk commented Sep 8, 2019 •
edited
Loading

I will also update the PR later to add a test.

Nice, there already is a test! And the test exposed a bug (the build failed when crateName != libName). I have fixed that in the buildRustCrate PR. Now build_and_run_bin_with_rerenamed_lib_dep also succeeds.

@danieldk danieldk mentioned this pull request Sep 8, 2019
Closed
@kolloch
Copy link
Collaborator

kolloch commented Sep 8, 2019

Thank you for working on this :)

@gilligan
Copy link

@danieldk The changes in nixpkgs have been merged. Thanks again. Now let's get this one over the finishing line :D

@danieldk danieldk marked this pull request as ready for review September 10, 2019 09:05
@danieldk
Copy link
Contributor Author

danieldk commented Sep 10, 2019 •
edited
Loading

I'll update this PR with the correct nixpkgs revision , then it should be ready for review.

@danieldk
Copy link
Contributor Author

nixpkgs revision bumped to NixOS/nixpkgs@a69a6c1

@gilligan
Copy link

@danieldk you could include a change to the README.md which at the moment still says that renaming isn't supported under the restrictions section ;}

@gilligan
Copy link

sweet, @kolloch can this be merged?

@treyfortmuller
Copy link

@kolloch The PR for nixpkgs has been merged to master (NixOS/nixpkgs#68296), should be ready to merge this PR

@jb55
Copy link

jb55 commented Sep 13, 2019 •
edited
Loading

oh I just hit this issue today, with this PR I get:

while evaluating the attribute 'buildDependencies' of the derivation 'rust_backtrace-sys-0.1.30' at /home/jb55/nixpkgs/pkgs/build-support/rust/build-rust-crate/default.nix:89:5:
while evaluating the attribute 'crateRenames' of the derivation 'rust_cc-1.0.26' at /home/jb55/nixpkgs/pkgs/build-support/rust/build-rust-crate/default.nix:89:5:
cannot coerce a set to a string, at /home/jb55/nixpkgs/pkgs/build-support/rust/build-rust-crate/default.nix:89:5

do I need something from nixpkgs upstream?

@jb55
Copy link

jb55 commented Sep 13, 2019 •
edited
Loading

oh I do, I need that from master. gotcha.

works great!

kolloch
kolloch approved these changes Sep 14, 2019
View reviewed changes
Copy link
Collaborator

@kolloch kolloch left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Great contribution :)

Could you add something to the CHANGELOG, mentioning your name? Otherwise, I'll do it after merge.

README.md
@@ -184,8 +184,6 @@ The enabled features for a crate now are resolved at build time! That means you
build generation time since we can simply pass this set to `cargo metadata`. Feature selection during build time is
out of scope for now.~~
* No support for building and running tests, see [nixpkgs, issue 59177](https://github.com/NixOS/nixpkgs/issues/59177).
* [Renamed crates](https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#renaming-dependencies-in-cargotoml)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please mention from what version on this is supported -- in line with the other issues? Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What version should I mention here? Do you plan to release 0.5.1 next or go to 0.6.0 (since renames introduce an API change in the form of extra arguments to buildRustCrate).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have now put Before 0.6.x here.

templates/build.nix.tera Outdated Show resolved Hide resolved
tests/sample_projects.rs
@@ -130,7 +130,6 @@ fn build_and_run_bin_with_lib_git_dep() {
}

#[test]
#[ignore]
fn build_and_run_bin_with_rerenamed_lib_dep() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome :)

@kolloch kolloch merged commit beaec46 into nix-community:master Sep 15, 2019
@danieldk danieldk deleted the renames branch September 19, 2019 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kolloch kolloch kolloch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support renamed crates
5 participants
@danieldk @kolloch @gilligan @treyfortmuller @jb55