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

fix: Revert vita's c_char back to i8 #4258

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

pheki
Copy link
Contributor

@pheki pheki commented Feb 5, 2025

Description

Hi,

#4199 changed the definition of c_char from i8 to u8 for most ARM targets. While that would usually be correct, VITASDK uses signed chars by default.

This will surely conflict with #4256, but I can rebase it after that one is merged. I opened that one first because in the off-chance it was just accepted, this fix would only be needed on rustc. I'll also create a rustc PR.

Sources

https://github.com/vitasdk/buildscripts/blob/master/patches/gcc/0001-gcc-10.patch#L33-L34

Checklist

  • Relevant tests in libc-test/semver have been updated (API didn't change)
  • No placeholder or unstable values like *LAST or *MAX are
    included (see #3131)
  • Tested locally (cd libc-test && cargo test --target mytarget);
    especially relevant for platforms that may not be checked in CI

@rustbot
Copy link
Collaborator

rustbot commented Feb 5, 2025

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@tgross35
Copy link
Contributor

tgross35 commented Feb 5, 2025

Thanks for the PR, but could you do the rustc change first? That's the better place to discuss this change and we can ping a few maintainers, libc will just follow whatever the decision is there.

@tgross35
Copy link
Contributor

tgross35 commented Feb 5, 2025

Also, does Clang do the right thing? This might need to be an upstream issue as well.

@pheki
Copy link
Contributor Author

pheki commented Feb 5, 2025

Thanks for the PR, but could you do the rustc change first? That's the better place to discuss this change and we can ping a few maintainers, libc will just follow whatever the decision is there.

Sure can, will do that.

Also, does Clang do the right thing? This might need to be an upstream issue as well.

It does not, but I'm not too worried as for now Clang is not really supported by the vita programming community / VITASDK. The pre-compiled libraries are all compiled using vita's gcc and we set TARGET_CC and TARGET_CXX in cargo vita for build scripts using cc. Regardless I'll take a look into fixing Clang once the Rust side is fixed.

@cuviper
Copy link
Member

cuviper commented Feb 12, 2025

Thanks for the PR, but could you do the rustc change first? That's the better place to discuss this change and we can ping a few maintainers, libc will just follow whatever the decision is there.

We're in agreement in rust-lang/rust#136667, so I think you can proceed here.

@tgross35 tgross35 added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Feb 14, 2025
@tgross35 tgross35 removed the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Feb 14, 2025
@tgross35
Copy link
Contributor

tgross35 commented Feb 14, 2025

As you mentioned above, this just needs a rebase

@rustbot author

@pheki pheki force-pushed the revert-vita-c-char branch from 23056e8 to e5a669b Compare February 15, 2025 03:16
@tgross35 tgross35 added this pull request to the merge queue Feb 15, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 15, 2025
@pheki
Copy link
Contributor Author

pheki commented Feb 16, 2025

Is it possible to retry the job in some way? The failure seems unrelated, the test for aarch64-linux-android hung

@rustbot ready

@tgross35 tgross35 added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Feb 17, 2025
@tgross35
Copy link
Contributor

Yeah, that isn't this PR's fault, restarted the job.

@tgross35 tgross35 added this pull request to the merge queue Feb 17, 2025
Merged via the queue into rust-lang:main with commit 780f817 Feb 17, 2025
44 checks passed
@pheki pheki deleted the revert-vita-c-char branch February 17, 2025 21:38
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Feb 19, 2025
(backport <rust-lang#4258>)
(cherry picked from commit e5a669b)
@tgross35 tgross35 mentioned this pull request Feb 19, 2025
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Feb 19, 2025
(backport <rust-lang#4258>)
(cherry picked from commit e5a669b)
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Feb 22, 2025
(backport <rust-lang#4258>)
(cherry picked from commit e5a669b)
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Feb 22, 2025
(backport <rust-lang#4258>)
(cherry picked from commit e5a669b)
@tgross35 tgross35 added stable-applied This PR has been cherry-picked to libc's stable release branch and removed stable-nominated This PR should be considered for cherry-pick to libc's stable release branch labels Feb 22, 2025
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 7, 2025
…viper

Revert vita's c_char back to i8

# Description

Hi!

rust-lang#132975 changed the definition of `c_char` from i8 to u8 for most ARM targets. While that would usually be correct, [VITASDK uses signed chars by default](https://github.com/vitasdk/buildscripts/blob/master/patches/gcc/0001-gcc-10.patch#L33-L34). The Clang definitions are incorrect because Clang is not (yet?) supported by the vita commmunity / `VITADSK`, On the Rust side, the pre-compiled libraries the user can link to are all compiled using vita's `gcc` and [we set `TARGET_CC` and `TARGET_CXX`](https://github.com/vita-rust/cargo-vita/blob/d564a132cbd43947118c0d6d0ebfbea7d1dd7fa7/src/commands/build.rs#L230) in `cargo vita` for build scripts using `cc`.

I'm creating it as a draft PR so that we can discuss it and possibly get it approved here, but wait to merge the [libc side](rust-lang/libc#4258) and get a libc version first, as having the definitions out of sync breaks std. As a nightly-only target it can be confusing/frustrating for new users when the latest nightly, which is the default, is broken.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 7, 2025
…viper

Revert vita's c_char back to i8

# Description

Hi!

rust-lang#132975 changed the definition of `c_char` from i8 to u8 for most ARM targets. While that would usually be correct, [VITASDK uses signed chars by default](https://github.com/vitasdk/buildscripts/blob/master/patches/gcc/0001-gcc-10.patch#L33-L34). The Clang definitions are incorrect because Clang is not (yet?) supported by the vita commmunity / `VITADSK`, On the Rust side, the pre-compiled libraries the user can link to are all compiled using vita's `gcc` and [we set `TARGET_CC` and `TARGET_CXX`](https://github.com/vita-rust/cargo-vita/blob/d564a132cbd43947118c0d6d0ebfbea7d1dd7fa7/src/commands/build.rs#L230) in `cargo vita` for build scripts using `cc`.

I'm creating it as a draft PR so that we can discuss it and possibly get it approved here, but wait to merge the [libc side](rust-lang/libc#4258) and get a libc version first, as having the definitions out of sync breaks std. As a nightly-only target it can be confusing/frustrating for new users when the latest nightly, which is the default, is broken.
jhpratt added a commit to jhpratt/rust that referenced this pull request Mar 7, 2025
…viper

Revert vita's c_char back to i8

# Description

Hi!

rust-lang#132975 changed the definition of `c_char` from i8 to u8 for most ARM targets. While that would usually be correct, [VITASDK uses signed chars by default](https://github.com/vitasdk/buildscripts/blob/master/patches/gcc/0001-gcc-10.patch#L33-L34). The Clang definitions are incorrect because Clang is not (yet?) supported by the vita commmunity / `VITADSK`, On the Rust side, the pre-compiled libraries the user can link to are all compiled using vita's `gcc` and [we set `TARGET_CC` and `TARGET_CXX`](https://github.com/vita-rust/cargo-vita/blob/d564a132cbd43947118c0d6d0ebfbea7d1dd7fa7/src/commands/build.rs#L230) in `cargo vita` for build scripts using `cc`.

I'm creating it as a draft PR so that we can discuss it and possibly get it approved here, but wait to merge the [libc side](rust-lang/libc#4258) and get a libc version first, as having the definitions out of sync breaks std. As a nightly-only target it can be confusing/frustrating for new users when the latest nightly, which is the default, is broken.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 7, 2025
Rollup merge of rust-lang#136667 - vita-rust:revert-vita-c-char, r=cuviper

Revert vita's c_char back to i8

# Description

Hi!

rust-lang#132975 changed the definition of `c_char` from i8 to u8 for most ARM targets. While that would usually be correct, [VITASDK uses signed chars by default](https://github.com/vitasdk/buildscripts/blob/master/patches/gcc/0001-gcc-10.patch#L33-L34). The Clang definitions are incorrect because Clang is not (yet?) supported by the vita commmunity / `VITADSK`, On the Rust side, the pre-compiled libraries the user can link to are all compiled using vita's `gcc` and [we set `TARGET_CC` and `TARGET_CXX`](https://github.com/vita-rust/cargo-vita/blob/d564a132cbd43947118c0d6d0ebfbea7d1dd7fa7/src/commands/build.rs#L230) in `cargo vita` for build scripts using `cc`.

I'm creating it as a draft PR so that we can discuss it and possibly get it approved here, but wait to merge the [libc side](rust-lang/libc#4258) and get a libc version first, as having the definitions out of sync breaks std. As a nightly-only target it can be confusing/frustrating for new users when the latest nightly, which is the default, is broken.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review stable-applied This PR has been cherry-picked to libc's stable release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants