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.

Sign up for GitHub

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

Jump to bottom

upgrade to emscripten 3.1.41 #3307

Closed
wants to merge 7 commits into from
Closed

upgrade to emscripten 3.1.41 #3307

wants to merge 7 commits into from

Conversation

benjamin-sieffert
Copy link

  • Increase size of time_t to 64-bit
  • Update values of a number of constants
  • Remove tests for a lot of headers that have been removed from emscripten
  • Bump emscripten version in test environment from 3.1.20 to 3.1.41 and use the recommended nodeJS version

Fixes #3306

@rustbot
Copy link
Collaborator

rustbot commented Jul 21, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) soon.

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

@benjamin-sieffert benjamin-sieffert mentioned this pull request Jul 21, 2023
Closed
@JohnTitor
Copy link
Member

We have to confirm that this change doesn't negatively affect std, marked as blocked until it's resolved.

@JohnTitor JohnTitor mentioned this pull request Aug 20, 2023
Merged
kleisauke added a commit to kleisauke/libc that referenced this pull request Aug 20, 2023
@kleisauke
Avoid conflicts with rust-lang#3307
78cee12 
This whole line is removed in that PR.
kleisauke
kleisauke reviewed Aug 20, 2023
View reviewed changes
ci/emscripten-entry.sh Outdated
@@ -7,6 +7,6 @@ source /emsdk-portable/emsdk_env.sh &> /dev/null

# emsdk-portable provides a node binary, but we need version 8 to run wasm
# NOTE: Do not forget to sync Node.js version with `emscripten.sh`!
export PATH="/node-v14.17.0-linux-x64/bin:$PATH"
export PATH="/node-v16.20.0-linux-x64/bin:$PATH"
Copy link
Contributor

Choose a reason for hiding this comment

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

This change can be omitted after PR #3321 lands.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! Adjusted

ci/emscripten.sh Outdated
@@ -24,5 +22,5 @@ chmod a+rxw -R /emsdk-portable
# node 8 is required to run wasm
# NOTE: Do not forget to sync Node.js version with `emscripten-entry.sh`!
cd /
curl --retry 5 -L https://nodejs.org/dist/v14.17.0/node-v14.17.0-linux-x64.tar.xz | \
curl --retry 5 -L https://nodejs.org/dist/v16.20.0/node-v16.20.0-linux-x64.tar.xz | \
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

@bors
Copy link
Contributor

bors commented Aug 21, 2023

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

@bors
Copy link
Contributor

bors commented Oct 6, 2023

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

JohnTitor
JohnTitor reviewed Jan 8, 2024
View reviewed changes
libc-test/build.rs
Comment on lines +2763 to +2766
// No personality.h etc.: https://github.com/emscripten-core/emscripten/pull/17704
// No sysctl.h: https://github.com/emscripten-core/emscripten/pull/18863
n if n.starts_with("PTRACE_") => true,
n if n.starts_with("QIF_") => true,
Copy link
Member

Choose a reason for hiding this comment

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

You should just remove the declaration instead of skipping.

@JohnTitor
Copy link
Member

It'd also be nice if you could rebase instead of merging and squashing commits into one.

@benjamin-sieffert
Copy link
Author

Sorry, I guess this is a bit over my head. I am too unfamiliar with all the details of what exactly is going on in the tests.
I tried moving this PR even further up to 3.1.44 and ran into issues I do not understand at all.
My motivation for this PR was fixing #3306 which causes Instant::now to have no sub-second precision on emscripten. (There is also an issue with certain std::fs functions crashing because of 32/64 bit mismatch on types, but I am not sure if this PR would have fixed that.)
For my use-case I have solved the timing issue by using emscripten_get_now instead.
I will abandon this PR and leave it to more involved devs to eventually figure out this emscripten/libc update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kleisauke kleisauke kleisauke left review comments

@JohnTitor JohnTitor JohnTitor left review comments

Assignees

@JohnTitor JohnTitor

Labels
breakage-candidate S-waiting-on-author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

emscripten time_t has wrong size
5 participants
@benjamin-sieffert @rustbot @JohnTitor @bors @kleisauke