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

c_char has changed on ARM in 1.6 #29867

Closed
alexcrichton opened this issue Nov 16, 2015 · 14 comments
Closed

c_char has changed on ARM in 1.6 #29867

alexcrichton opened this issue Nov 16, 2015 · 14 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

Android was changed in #29546 and Linux ARM was changed in #29775. Both are bug fixes as the definitions have been corrected, but this is causing breakage:

Can we help mitigate this breakage? Should we revert "the fix" for now?

Opening a tracking issue (and nominating it) to discuss this.

@alexcrichton alexcrichton added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 16, 2015
@alexcrichton
Copy link
Member Author

Some other points:

  • Updating to libc 0.2 and using Rust nightly should not cause errors to show up
  • ARM is not a tier 1 platform right now, but it's kinda close to one
  • libc 0.1 is no longer compatible with nightly std on ARM (due to this and CString)
  • libc 0.2 is not compatible with Rust 1.5 and prior (due to this and CString)

@arcnmx
Copy link
Contributor

arcnmx commented Nov 16, 2015

Note that #29775 only changed raw::os::c_char to be consistent, which probably no one seemed to be using at all. ARM Linux was also broken in #29546 since the main observable problem was with the type CStr::as_ptr returned.

It sucks. My solution has been to pin specific versions via hacking on Cargo.lock. I'm not sure that reverting back to the wrong type is the right course of action to take, though.

@SimonSapin
Copy link
Contributor

Should library use types from std::os::raw when they exist instead of libc? (Maybe depending on whether or not they need other things from libc?) Should rust-bindgen be changed to use std::os::raw for types that exist there?

@alexcrichton
Copy link
Member Author

That's a good question! I don't think it should matter as the types should in theory always be the same.

@arcnmx
Copy link
Contributor

arcnmx commented Nov 16, 2015

I think they should, tbh. In theory they should be the same, but if you plan on using std methods, you should be using the types it exports, and not assuming some external crates.io crate matches up.

For bonus points: make them quasi-newtypes so a cast between the two is required :P

Also: All Android platforms (including x86) seem to be affected.

@SimonSapin
Copy link
Contributor

CC @larsbergstrom

@bluss
Copy link
Member

bluss commented Nov 17, 2015

Can we reorganize this somehow so that we use a single source of truth? Defining c_char in libstd, in-tree liblibc and crates.io liblibc is in two places too many right.

@alexcrichton
Copy link
Member Author

The libs team discussed this during triage yesterday and the conclusion was that we're unlikely to go back to the old incorrect definitions and otherwise we'll basically just do what we can to help mitigate this breakage.

@bluss that's possible, yeah, although it wouldn't have fixed this problem because the "single source of truth" from before was wrong (both libstd and libc disagreed).

@SimonSapin
Copy link
Contributor

I hope that updating all the things (servo/servo#8608) to libc 0.2 will fix the build errors related to this for Servo.

@bluss
Copy link
Member

bluss commented Nov 19, 2015

@alexcrichton But the major fallout comes from incompatible types, not from the change per se? If all the crate including libc used c_char reexported from libstd, and I think you can't compile against more than one version of libstd, they'd all be using the same type.

@alexcrichton
Copy link
Member Author

Yeah that's true and it would help mitigate problems like this. There's a number of interesting issues around libc reexporting types from the standard library, however, so it's currently not done.

@cuviper
Copy link
Member

cuviper commented Nov 19, 2015

@bluss but libc is the source of c_char for libstd, so the problem comes when your external libc crate doesn't match the one built into libstd. (edit: I take that back, raw::c_char is defined on its own. TIL)

Maybe the libc crate should use a cfg feature for this? Set that feature to provide its own good c_char when used within libstd, else just reexport std::os::raw::c_char. This way external use always agrees with libstd.

@MagaTailor
Copy link

hannobraun/inotify-rs#33

@MagaTailor
Copy link

Even aarch64 is affected to this very day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants