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(wasi): Add back unsafe block for clockid_t static variables #4157

Merged
merged 2 commits into from
Nov 27, 2024

Conversation

DarumaDocker
Copy link
Contributor

The MSRV is still far before 1.82.0

Description

Version 0.2.166 can not be built for the wasm32-wasi target caused by the removed unsafe block.
This commit is taking them back.

Checklist

  • Relevant tests in libc-test/semver have been updated
  • 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 Nov 27, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tgross35 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@tgross35
Copy link
Contributor

Is the MSRV not getting checked in CI on wasi? I'm not sure why this would be failing.

Please target main and run ./ci/style.sh to fix formatting, change lgtm.

@tgross35
Copy link
Contributor

tgross35 commented Nov 27, 2024

It looks like the target just didn't get added to https://github.com/rust-lang/libc/blob/511e3f2cb7e4ca24ffbe0ce33879faf733717228/ci/verify-build.sh. Could you add it here? It will need to check if "$rust" = "1.63" and then, if so rename wasm32-waspi1 to wasm32-wasi and skip wasm32-wasip2, but I'm not even sure if we can expect to keep the same MSRV for WASI considering it's had quite a few changes since 1.63.

Cc @alexcrichton if you have any better ideas here.

@tgross35 tgross35 changed the title fix: Take back unsafe block for clockid_t static variables fix(wasi): Add back unsafe block for clockid_t static variables Nov 27, 2024
@tgross35 tgross35 added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Nov 27, 2024
@tgross35
Copy link
Contributor

@DarumaDocker could you please target the main branch?

@DarumaDocker
Copy link
Contributor Author

@DarumaDocker could you please target the main branch?

Do you mean I should fork from main and create a new PR?

@tgross35
Copy link
Contributor

Just rebase onto main and force push the same PR, github lets you change the target branch (click edit by the title)

Comment on lines 242 to 248
if [ "$rust" = "stable" ] || [ "$rust" = "nightly" ]; then
test_target "$target"
else
if [ "$target" = "wasm32-wasip1" ]; then
test_target "wasm32-wasi"
elif [ "$target" != "wasm32-wasip2" ]; then
test_target "$target"
fi
fi
Copy link
Contributor

@tgross35 tgross35 Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be cleaner as a case statement

case "$target" in
    "stable") supports_wasi_pn=1 ;;
    "beta") supports_wasi_pn=1 ;;
    "nightly") supports_wasi_pn=1 ;;
    *) supports_wasi_pn=0 ;;
esac

# `wasm32-wasip1` was renamed from `wasm32-wasi`
if [ "$target" = "wasm32-wasip1" ] && [ "$supports_wasi_pn" = "0" ]; then
    target="wasm32-wasi"
fi

# `wasm32-wasip2` only exists in recent versions of Rust
if [ "$target" = "wasm32-wasip2" ] && [ "$supports_wasi_pn" = "0" ]; then
    continue
fi

test_target "$target"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Nov 27, 2024
(backport <rust-lang#4157>)
(cherry picked from commit 5fc0321)
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Nov 27, 2024
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Nov 27, 2024
(backport <rust-lang#4157>)
(cherry picked from commit 5fc0321)
@tgross35
Copy link
Contributor

Released in 0.2.167

@alexcrichton
Copy link
Member

I'm not even sure if we can expect to keep the same MSRV for WASI considering it's had quite a few changes since 1.63.

Personally I'd consider it reasonable to have a different MSRV for certain targets, for example WASI. It looks like this got worked out in the end though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CI Area: CI-related items O-wasi 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