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

Remove cloudabi, winapi, and fuchsia-cprng dependancies #40

Merged
merged 6 commits into from
Jun 27, 2019

Conversation

josephlr
Copy link
Member

Depends on #39 and helps address issues raised in rust-lang/rust#62082 (comment)

We don't really need these external crates as they just provide declarations of system functions that we can just extern declare ourselves.

This PR also updates the docs and cleans up Cargo.toml to explain why we need lazy_static on certain targets.

This also fixes the fuchsia CI issues.

@newpavlov
Copy link
Member

I also prefer to keep those extern's inside getrandom instead of depending on platform crates, but @dhardy had a different opinion at the time.

@dhardy
Copy link
Member

dhardy commented Jun 26, 2019

I'm not too fussed; seems sensible enough to avoid the dependencies in this case.

@newpavlov
Copy link
Member

Can you fix the CI failure and rebase (otherwise right now PR contains formatting changes)?

@josephlr
Copy link
Member Author

Can you fix the CI failure and rebase (otherwise right now PR contains formatting changes)?

Done

@@ -19,33 +19,20 @@ members = ["tests/wasm_bindgen"]

[dependencies]
log = { version = "0.4", optional = true }
libc = "0.2.54"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want to depend on libc for all targets. Maybe instead construct one big cfg expression with enumeration of targets which use libc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, I just used unix || wasi. Although that's not perfect, as we don't use libc on all unix targets (only 7 of them), but writing all that out makes the Cargo.toml way harder to read.

Copy link
Member

Choose a reason for hiding this comment

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

We also could remove libc dependency for WASI as well, libc code is quite straightforward. Not sure if it's worth though.

Copy link
Member Author

Choose a reason for hiding this comment

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

We totally could (Strictly speaking we could do it for any libc declaration), but I think it’s best to not duplicate declarations from the libc, if possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we wanted we could not depend on the libc at all for any target, by copying over the 6 or 7 libc declarations needed to make everything link. But I think that would make breakage/instability more likely, not less.

Copy link
Member

@newpavlov newpavlov Jun 27, 2019

Choose a reason for hiding this comment

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

It's just a bit strange for me to depend on libc crate for a target which, well, does not have libc as a system interface, i.e. it's defined in terms of core functions. (I wonder why WASI target was added to libc crate in the first place, instead of creating a separate crate à la cloudabi or Fuchsia crates)

Copy link
Member Author

Choose a reason for hiding this comment

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

So I think they added it because wasi-libc defines that symbol. However, it's just a macro that links to the wasi_unstable module. From their Github issues, it seems like the wasi_unstable module is an implementation detail and may change in the future.

Given that, linking to the libc seems like the most stable approach, so we are not depending on an unstable components of the WASI API.

Copy link
Member

@newpavlov newpavlov Jun 27, 2019

Choose a reason for hiding this comment

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

In my understanding we shouldn't care about wasi-libc at all. WASI defines Core API and __wasi_random_get is part of it. So we should be able to use it directly without any issues.

cc @alexcrichton @gnzlbg

UPD: Ah, the linked page states that:

The function names are prefixed with "_wasi" to reflect how they are spelled in flat-namespace contexts, however at the wasm module level, they are unprefixed, because they're inside a module namespace (currently "wasi_unstable").

So it looks like random_get (to which __wasi_random_get links) is unstable. I don't quite get this situation, but it seems that using libc will be indeed the best option right now.

src/windows.rs Outdated Show resolved Hide resolved
@dhardy
Copy link
Member

dhardy commented Jun 27, 2019

Can I leave this to you @newpavlov and @josephlr?

@newpavlov
Copy link
Member

Yes, I will do the merge after we will decide if we want to depend on libc for WASI target.

@newpavlov newpavlov merged commit ea999a6 into rust-random:master Jun 27, 2019
@josephlr josephlr deleted the dps branch June 28, 2019 19:33
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

Successfully merging this pull request may close these issues.

None yet

3 participants