emscripten: Use the latest emsdk 3.1.68

[workingjubilee]

This should fix our precompiled std being unsound in std::fs code.

Should resolve #131467 I hope.

[rustbot]

r? @Kobzol

rustbot has assigned @Kobzol.
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

[Kobzol]

I have to say that I'm often clueless when reviewing these kinds of PRs :) Do we have some documentation about what does it mean to update this? Do we even document somewhere which version of emscripten we use for given targets, or is it just an "internal implementation detail".

[workingjubilee]

I have to say that I'm often clueless when reviewing these kinds of PRs :) Do we have some documentation about what does it mean to update this? Do we even document somewhere which version of emscripten we use for given targets, or is it just an "internal implementation detail".

We do not, but we should, probably. I can add the relevant details for our platform support pages?

I think since we build the SDK, it shouldn't have a significant consequence? (aside from the bugfix, of course)

I am not entirely sure what the consequences of this are, tbh. I'm also not entirely sure this won't break Pyodide for @juntyr, either, given 3.1.68 is not 3.1.46, but I'm not sure what to make of emsdk's versioning. It could be SemVer, in which case this should be fine, but then why did 3.1.42 break ABI?

Perhaps @alexcrichton or @Manishearth might know?

[juntyr]

Updating the Emscripten version to something newer is definitely a good idea. But it should go hand-in-hand with some documentation (I'll submit a PR later) on this coupling between Emscripten and Rust that won't matter in most cases until you get a weird bug, and how to work around it with -Zbuild-std.

I think I was experiencing this issue for a long while but simply never noticed. Pyodide can use ~2GB RAM before anything breaks, and up until a few days ago even the senseless overallocation during file reading never hit the boundary. So it was by pure chance and additional up-front memory pressure that allocating an extra 1.8GB was no longer fine and I noticed the ABI mismatch.

[hoodmane]

Thanks for helping to support the Emscripten target @workingjubilee! I really appreciate it.

[alexcrichton]

Alas I'm ignorant in the ways of Emscripten here so I also don't understand the full consequences of what an update here might have. In the absence of more information it seems reasonable to go ahead with the update and use it as a learning process for what to and what not to do in the future.

[workingjubilee]

atm I don't think anyone understands how Rust interacts with Emscripten, tbh.

@sbc100 if you could weigh in?

[sbc100]

I also don't know much about how emscripten and rust work together, but updating certainly seems like a good idea. 2.0.5 is ancient.

I assume there is some amount of CI testing that occurs here to make sure at least some things work?

[workingjubilee]

I also don't know much about how emscripten and rust work together, but updating certainly seems like a good idea. 2.0.5 is ancient.

I assume there is some amount of CI testing that occurs here to make sure at least some things work?

...we make sure things build! :D

[workingjubilee]

Only a tier 1 target is actually expected to run tests, and they run all the tests, so tier 2 targets like emscripten Basically Don't. There's a few which run anyways, but we do not extensively test tier 2 non-host targets. In fact there were quite a few ignores in the test suite just to make sure that no one even tries to run them for emscripten. Some of that was unavoidable because e.g. emscripten doesn't have enough IO support and some tests are of IO-using programs.

But uh, I am the very person who ripped out asmjs-unknown-emscripten support because it had been silently broken for years, as the only vestiges of CI testing that we have for tier 2 had been completely disabled for it, and no one noticed or complained.

[Kobzol]

It seems like merging this and seeing if it breaks anyone externally might be the only reasonable way forward (assuming that it passes CI). You can r=me in that regard.

[workingjubilee]

Yeah, that was my feeling as well.

It would be nice to land more tests for emscripten but even for the tests that we do run, it already has a custom test harness in compiletest. More tests may thus require wrangling more Fun, and such tests will not necessarily be checked in CI. Indeed, a large portion of the UI test suite seems to fall prey to linking failures when I try to build it, for reasons that don't have any clear relationship with the EMSDK version, and probably have been true for quite some time.

As this is a soundness concern, it doesn't seem worth blocking on a task which will take an indefinite amount of time. Thus, let us give scream-based deployment testing a go.

@bors r=Kobzol

[bors]

📌 Commit 2c38ecf has been approved by Kobzol

It is now in the queue for this repository.