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 WASI Core API #1461

Merged
merged 3 commits into from Sep 7, 2019

Conversation

@newpavlov
Copy link
Contributor

commented Aug 9, 2019

Closes #1434

This change does not break the backwards compatibility promise since WASI Core API is unstable right now. If applications or libraries want to use Core API directly they should use wasi instead of libc.

Blocked by: rust-lang/rust#63676

cc @sunfishcode

@rust-highfive

This comment has been minimized.

Copy link

commented Aug 9, 2019

r? @gnzlbg

(rust_highfive has picked a reviewer for you, use r? to override)

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

✌️ @sunfishcode can now approve this pull request

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2019

@sunfishcode this needs your review
cc @alexcrichton maybe?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

I do not agree with this change and would not remove this API. Instability inherent with a platform will of course cause instability in the libraries for the platform, and no one's expecting the libc crate to make a breaking change when the wasi api changes.

@newpavlov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@alexcrichton
Do you believe that libc is a correct place for exposing (unstable) WASI Core API, even considering the existence of the dedicated wasi crate? Or are you hesitant to introduce a breaking change with such removal?

If it's the latter, I guess we could mark those functions deprecated for the time being, but I think eventually WASI Core API should be removed from libc.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Yes, I believe libc is the right place for these APIs. They're in the header files in the C implementation, so libc seems like the place to put them.

I would also not go as far as marking them deprecated. We'll just follow what wasi does in C, and if that breaks then the Rust target will break. Again no one is expecting the libc crate to have a breaking version bump for the wasi target. Everything about wasi is unstable and breaking and changing.

@sunfishcode

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

This path is growing on me. The reason these things on the C side are declared in WASI libc is mainly that we don't have a package manager for C so it's convenient to have everything live in one place. But the WASI interfaces are logically a layer below the rest of libc.

@newpavlov This patch should probably also remove the various __wasi_* types. And, what would you think about removing all the various __WASI_* constants too, and importing them from the wasi crate, as needed, instead?

@newpavlov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@sunfishcode
Ah, yes, I have indeed overlooked __wasi_* types. I am not sure about importing constants from wasi though. Can't we simply duplicate them? For example for Fuchsia libc duplicates constants instead of importing them from Fuchsia crates.

@newpavlov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

I have removed __wasi_* types and __WASI_* constants, but left __wasilibc_* functions untouched.

sunfishcode added a commit to CraneStation/wasi-misc-tests that referenced this pull request Aug 13, 2019
Use the wasi crate.
Switch from depending on libc to depending on the new wasi crate to provide
the low-level WASI interfaces.

See also rust-lang/libc#1461.
@sunfishcode sunfishcode referenced this pull request Aug 13, 2019
@sunfishcode

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Overall this looks good.

I'm wondering if there's anything else we can do about the __WASI_* constants though. WASI is less stable than Fuchsia at this point, and some of these constants could very well change at the WASI level. Besides the awkwardness for maintainers to keep everything in sync, it's awkward for end users if eg __WASI_O_CREAT ever changes, because then their open(..., O_CREAT) code would break. It doesn't say which WASI version it's using. But, I'm not sure if there's anything we can do about that.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

I'm fine deferring to @sunfishcode's judgement here, but to avoid another debacle of updating libc in rust-lang/rust I'd request that the reliance of libstd on these symbols is removed before this PR is merged.

@newpavlov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

@sunfishcode
Can you add removed types and constants to wasi? After that I can help with migrating rust-lang/rust to wasi if you want.

@sunfishcode

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

@newpavlov The current wasi package has all the types and constants in it.

@newpavlov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

@sunfishcode What should I do with __wasilibc_find_relpath? It uses __wasi_rights_t, should I duplicate it in libc?

sunfishcode added a commit to CraneStation/wasi-misc-tests that referenced this pull request Aug 19, 2019
Use the wasi crate.
Switch from depending on libc to depending on the new wasi crate to provide
the low-level WASI interfaces.

See also rust-lang/libc#1461.
@sunfishcode

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Yeah, I think it's fine to duplicate the definition of __wasi_rights_t here for now. That said, it may change, but that's just another instance of a more general problem.

Centril added a commit to Centril/rust that referenced this pull request Sep 5, 2019
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 6, 2019
bors added a commit to rust-lang/rust that referenced this pull request Sep 6, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 6, 2019
@newpavlov

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

rust-lang/rust#63676 has landed, so this PR is not blocked anymore.

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

commented Sep 7, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

📌 Commit 078a748 has been approved by gnzlbg

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

⌛️ Testing commit 078a748 with merge fdbcc11...

bors added a commit that referenced this pull request Sep 7, 2019
Auto merge of #1461 - newpavlov:patch-4, r=gnzlbg
Remove WASI Core API

Closes #1434

This change does not break the backwards compatibility promise since WASI Core API is unstable right now. If applications or libraries want to use Core API directly they should use [`wasi`](https://crates.io/crates/wasi) instead of `libc`.

Blocked by: rust-lang/rust#63676

cc @sunfishcode
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

☀️ Test successful - checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, status-azure
Approved by: gnzlbg
Pushing fdbcc11 to master...

@bors bors merged commit 078a748 into rust-lang:master Sep 7, 2019

3 of 4 checks passed

nightly x86_64-unknown-freebsd-12 Task Summary
Details
homu Test successful
Details
rust-lang.libc #20190821.8 succeeded
Details
stable x86_64-unknown-freebsd-11 Task Summary
Details

@newpavlov newpavlov deleted the newpavlov:patch-4 branch Sep 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.