Skip to content

Factor out platforms for which libc is empty #1126

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

Merged
merged 1 commit into from
Nov 20, 2018

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Nov 19, 2018

This change shouldn't change any functionality. It just separates the platforms for which libc is currently empty (only wasm32-unknown-unknown), from those for which it isn't. This is a non-functional change.

@rust-highfive
Copy link

r? @alexcrichton

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

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 19, 2018

Note that libc does not export c_void for wasm32-unknown-unknown, and that c_void is part of libcore.

@alexcrichton
Copy link
Member

r=me, but looks like ci may be broken?

We'll fix this eventually for wasm32-unknown-unknown once the Clang ABI stabilizes and/or is released, but until that time we're gonna leave this as empty

@jethrogb jethrogb mentioned this pull request Nov 19, 2018
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 19, 2018

Yes, so summarizing the conversation from IRC, libc should always compile for all targets, but it should be empty for targets without a libc (or similar).

That is, wasm32-unknown-unknown would check that box at the moment (we can change that in the future), and the target_env = "none" targets would do so as well.

So i'm going to keep this PR more or less as is until its merged, but at some point we might want to go from explicitly listing the non-supported target, to "somehow" only have items in the supported targets and be empty by default.

@bors
Copy link
Contributor

bors commented Nov 20, 2018

☔ The latest upstream changes (presumably #1127) made this pull request unmergeable. Please resolve the merge conflicts.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 20, 2018

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 20, 2018

📌 Commit d145731 has been approved by gnzlbg

@bors
Copy link
Contributor

bors commented Nov 20, 2018

⌛ Testing commit d145731 with merge 16f6fef...

bors added a commit that referenced this pull request Nov 20, 2018
Factor out platforms for which libc is empty

This change shouldn't change any functionality. It just separates the platforms for which `libc` is currently empty (only `wasm32-unknown-unknown`), from those for which it isn't. This is a non-functional change.
@bors
Copy link
Contributor

bors commented Nov 20, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: gnzlbg
Pushing 16f6fef to master...

@bors bors merged commit d145731 into rust-lang:master Nov 20, 2018
@alexcrichton
Copy link
Member

I think this unfortunately breaks compilation of the wasm target in upstream rust-lang/rust, it turns out that the #![cfg] on the crate also removes the attributes of the crate! You can see that via:

// foo.rs
#![feature(no_core)]
#![no_core]
#![cfg(foo)]

compiled with

$ rustc foo.rs --crate-type lib --target x86_64-pc-windows-msvc
error[E0463]: can't find crate for `std`
  |
  = note: the `x86_64-pc-windows-msvc` target may not be installed

error: aborting due to previous error

For more information about this error, try `rustc --explain E0463`.

(it shouldn't try to link std)

@alexcrichton
Copy link
Member

I believe #1129 fixes that though

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 21, 2018

I believe #1129 fixes that though

Damn, I'm so sorry. I was just going to ask whether that PR fixes it.

@alexcrichton
Copy link
Member

No worries! I had no idea about this as well :)

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 21, 2018

I'm trying to get rust-semverver up to a point that can be used to detect breaking changes to libc.

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.

4 participants