-
Notifications
You must be signed in to change notification settings - Fork 198
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
Conversation
I also prefer to keep those |
I'm not too fussed; seems sensible enough to avoid the dependencies in this case. |
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" |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
Right now a subset of unix targets and wasi use it. Note that some Unix targets (like macOS) don't need to use it.
Also, reenable fuchsia building
Can I leave this to you @newpavlov and @josephlr? |
Yes, I will do the merge after we will decide if we want to depend on |
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 needlazy_static
on certain targets.This also fixes the fuchsia CI issues.