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

Problems understanding getrandom_package #881

Closed
gilligan opened this issue Sep 3, 2019 · 9 comments
Closed

Problems understanding getrandom_package #881

gilligan opened this issue Sep 3, 2019 · 9 comments

Comments

@gilligan
Copy link

gilligan commented Sep 3, 2019

Hi, I'm a Nix user and recently the Rust/Nix tools i'm using (like crate2nix) broke because of changes introduced in rand - In the end it probably just boils down to missing renaming support and of course this is none of your concern/responsibility.

However I would be really grateful if you could help me to understand how you are using getrandom / getrandom_package because i am still confused about it.


The first thing I dug up was this comment #800 (comment)

Optional dependencies can be used as feature flags, but I don't think
it's possible to make them depend on anything transitively. I want
getrandom to imply "rand_core/getrandom"; this is a hack to do that.

This of course refers to the following setup in the Cargo.toml:

[features]
getrandom = ["getrandom_package", "rand_core/getrandom"]

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

So rand is exposing a getrandom feature which introduces a dependency on getrandom_package. getrandom_package (which doesn't actually exist anywhere) is mapped/renamed to use getrandom. In addition to that the getrandom feature also introduces a dependency on rand_core/getrandom.

It would be great if someone could explain the motivation behind this to me. Couldn't this just be replaced with something like

[features]
random = ["getrandom", "rand_core/getrandom"]
# getrandom = ["getrandom", "rand_core/getrandom"] circular dep problem (?)

Is the hack necessary to avoid some otherwise occurring breakage? I'd be most grateful if someone could explain this to me. I'm sorry if this is entirely obvious, i am not that experienced with Rust yet.

Thanks a lot in advance folks.

@vks
Copy link
Collaborator

vks commented Sep 3, 2019

I think the renaming is necessary to have a feature called getrandom. Renaming it to random would probably be work as well, but the name is less clear.

getrandom = ["getrandom", "rand_core/getrandom"] circular dep problem (?)

This is not possible, because features and dependencies cannot share the same name.

@gilligan
Copy link
Author

gilligan commented Sep 3, 2019

@vks well but @dhardy explains the renaming as a "hack" to introduce a transitive dependency and i'm still confused about that part 🤔

@dhardy
Copy link
Member

dhardy commented Sep 3, 2019

It is exactly that, a hack because of the way that Cargo works. I didn't want to rename the "feature" from getrandom because that would have been a breaking change — that was already available (previously as an optional package, but from the user's point of view equivalent to a feature) before the rand_core crate gained a getrandom feature.

My understanding is that you're unhappy because crate2nix doesn't quite support the same functionality as Cargo and this hack happened to fall into one of these gaps? Sorry about that, but I don't believe there's much we can do about it. (My view is that Cargo should never have conflated features and optional dependencies in the first place.)

@gilligan
Copy link
Author

gilligan commented Sep 3, 2019

@dhardy thanks a lot for your clarification. Indeed i was only hoping for a clarification because i need to adjust the tooling. In this case adding support for renaming which wasn’t added initially.

Thanks again!

@gilligan gilligan closed this as completed Sep 3, 2019
@treyfortmuller
Copy link

@gilligan have you found a solution for this problem? I am also a Nix user and have tried building my Rust package (which has this rand crate as an optional dependency of a dependency) through carnix, nix-build (which should just be calling Cargo wholesale), and the mozilla-nixpkgs overlays. All three approaches fail on the following error:

error[E0432]: unresolved import `getrandom_package`
  --> src/lib.rs:60:5
   |
60 | use getrandom_package as getrandom;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `getrandom_package` external crate

error[E0425]: cannot find function `getrandom` in this scope
  --> src/rngs/os.rs:78:9
   |
78 |         getrandom(dest)?;
   |         ^^^^^^^^^ not found in this scope
help: possible candidate is found in another module, you can import it into scope
   |
12 | use libc::getrandom;

@dhardy can you recommend a way to avoid this optional dependency/renaming scheme from destroying the Nix workflow? I don't actually depend on rand but nalgebra, cbindgen and a lot of other crates I use depend on rand. I'm experimenting with forking rand, removing this renaming scheme, and applying a patch to rand in my crate's Cargo.toml. Is this a possible avenue forward? I appreciate any help!

@dhardy
Copy link
Member

dhardy commented Sep 8, 2019

Well, I didn't forsee this hack breaking the nix tooling. @gilligan are you proposing to fix the tooling?

Removing this getrandom_package rename would also disable the feature forwarding. I guess if we make std depend on rand_core/getrandom instead then most users would be okay, but no_std users using getrandom (e.g. SGX target) would be affected.

@dhardy dhardy reopened this Sep 8, 2019
@gilligan
Copy link
Author

gilligan commented Sep 8, 2019

The good news is that there is ongoing work to fix this already. There is a PR for nixpkgs which will be merged soon: NixOS/nixpkgs#68296

Ultimately i think rand authors shouldn’t have to do anything because renaming per se is of course a valid Cargo feature.

If you take a look at nix-community/crate2nix#22 (comment) you can see how you can patch this for now.

TL;DR everything should soon be fixed on the nix side of things.

@danieldk
Copy link

danieldk commented Sep 8, 2019

If you take a look at kolloch/crate2nix#22 (comment) you can see how you can patch this for now.

I have also made a PR in crate2nix to make use of NixOS/nixpkgs#68296:

nix-community/crate2nix#24

Any testing would be appreciated ;).

@dhardy
Copy link
Member

dhardy commented Sep 27, 2019

Your PR is merged so I assume this has been sorted now.

@dhardy dhardy closed this as completed Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants