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

Rename wasm32-wasi to wasm32-wasi-preview1 #110596

Closed
wants to merge 14 commits into from

Conversation

yoshuawuyts
Copy link
Member

@yoshuawuyts yoshuawuyts commented Apr 20, 2023

@rustbot
Copy link
Collaborator

rustbot commented Apr 20, 2023

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 20, 2023
@WaffleLapkin WaffleLapkin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 20, 2023
@WaffleLapkin
Copy link
Member

Marking as waiting on author, given the WIP state; feel free to ping me/change labels when ready.

@yoshuawuyts yoshuawuyts force-pushed the rename-wasm32-wasi branch 2 times, most recently from 81f2641 to a5af9d1 Compare April 20, 2023 13:52
@rust-log-analyzer

This comment has been minimized.

@yoshuawuyts yoshuawuyts marked this pull request as ready for review April 20, 2023 16:17
@rustbot
Copy link
Collaborator

rustbot commented Apr 20, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

These commits modify compiler targets.
(See the Target Tier Policy.)

The Miri subtree was changed

cc @rust-lang/miri

@yoshuawuyts
Copy link
Member Author

This compiles locally now, and CI seems to be passing!

@WaffleLapkin can I ask you to maybe take a look now?

Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

I think the MCP mentioned the period where the old name would be accepted with the new name, but it looks like this PR just renames the target, without handling the old name in any way, why so?

Additionally I've found a few mentions of wasm32-wasi in the repo, that were not changed:

library/stdarch/ci/docker/wasm32-wasi/Dockerfile
15:  --mapdir .::/checkout/target/wasm32-wasi/release/deps

src/stage0.json
224:    "dist/2023-03-07/rust-std-beta-wasm32-wasi.tar.gz": "265fa8b315a5d39a35fb8d32d5e46c3c66f9608992a3d708ac90437818cfed45",
225:    "dist/2023-03-07/rust-std-beta-wasm32-wasi.tar.xz": "26839f3ed020dbda8ba893492cf504d565e7e1af7cfec5ad76053443d1022839",

src/doc/rustc-dev-guide/src/building/how-to-build-and-run.md
221:and your cross-compilation target is `wasm32-wasi`, you can build with:
224:./x.py build --target x86_64-unknown-linux-gnu --target wasm32-wasi
236:target = ["x86_64-unknown-linux-gnu", "wasm32-wasi"]
257:cargo +stage1 build --target wasm32-wasi

Can you clarify why these are not/should not be changed?

config.example.toml Outdated Show resolved Hide resolved
@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Apr 21, 2023

I think the MCP mentioned the period where the old name would be accepted with the new name, but it looks like this PR just renames the target, without handling the old name in any way, why so?

@WaffleLapkin This is something we explicitly decided not to do. I think perhaps you might be confusing it with both Preview 1 and Preview 2 existing in the compiler for the foreseeable future? That was in the Preview 2 MCP. Relevant text from the Preview 1 rename MCP:

Because we are not (yet) removing a target but only renaming it, we should be able to do this over a regular Rust release. This means we are not going to have a period where the wasm32-wasi and wasm32-wasi-preview1 targets are both available in the toolchain.


Additionally I've found a few mentions of wasm32-wasi in the repo, that were not changed:

Oops! Thank you for spotting that! - The first and third one are reason other than I don't remember it showing up when I grepped the codebase.

The second one seems to be describing some sort of mapping of hashes and targets though. I actually don't know what that is, or how I should update that? Do I just rename the target, and will the hash remain the same? I could just try it and see what happens? I'll ask the bootstrap team on Zulip what to do with this.

@yoshuawuyts
Copy link
Member Author

I'll ask the bootstrap team on Zulip what to do with this.

I asked the bootstrap team, and they told me not to touch src/stage0.json. It will get auto-regenerated when the next beta release is cut - so we don't need to do anything with that right now.

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Apr 21, 2023

Okay, I've tracked down the other two missing renames of wasm32-wasi, and I need to update different repos for that. Namely:

I'll file separate patches to both those repos. And I think with that all feedback from review has been handled.

edit: filed the PRs and linked them in the list.

@yoshuawuyts yoshuawuyts changed the title Rename wasm32-wasi target to wasm32-wasi-preview1 Rename wasm32-wasi to wasm32-wasi-preview1 Apr 21, 2023
@WaffleLapkin
Copy link
Member

Relevant text from the Preview 1 rename MCP

Yeah, oops. I totally missed the "not" there 😅

Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

r=me with the last nitpick fixed

config.example.toml Outdated Show resolved Hide resolved
@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Apr 21, 2023

@WaffleLapkin thanks for catching that; fixed! I think that means this can now be merged 😊

@klensy
Copy link
Contributor

klensy commented Apr 21, 2023

I understand correct, this renames wasm32-wasi to wasm32-wasi-preview1, without adding alias between them (at least temporary, to prevent breakage) and intentionally breaking documentation all over internet.

@wesleywiser
Copy link
Member

@yoshuawuyts Can you write a blog for the Rust blog announcing this change? It would be great if we can coordinate those together with this.

@jyn514 jyn514 reopened this Jul 12, 2023
@bors
Copy link
Contributor

bors commented Jul 12, 2023

⌛ Trying commit 734c8c9 with merge 14b728e7b91e4d13222d1fec97b0ed7ae14041dc...

@wesleywiser
Copy link
Member

(it's still marked as "trying" in the bors queue even after @workingjubilee closed and reopened. Trying again...)

@wesleywiser wesleywiser reopened this Jul 13, 2023
@wesleywiser
Copy link
Member

Wait... now it's "pending" not "pending (try)"

@bors r-

@wesleywiser
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Jul 13, 2023

⌛ Trying commit 734c8c9 with merge e1e8b3f26b4661e6bcda6a18408e8fc8b5ed2d8a...

@bors
Copy link
Contributor

bors commented Jul 13, 2023

☀️ Try build successful - checks-actions
Build commit: e1e8b3f26b4661e6bcda6a18408e8fc8b5ed2d8a (e1e8b3f26b4661e6bcda6a18408e8fc8b5ed2d8a)

@wesleywiser
Copy link
Member

Hi @Mark-Simulacrum, we've gotten the try build sorted out and completed. Could you kick off a dev-static release again? Thanks!

@wesleywiser wesleywiser added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 17, 2023
@@ -306,7 +306,7 @@ fn copy_self_contained_objects(
.unwrap_or_else(|| {
panic!("Target {:?} does not have a \"wasi-root\" key", target.triple)
})
.join("lib/wasm32-wasi");
.join("lib/wasm32-wasi-preview1");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it able to find wasi-libc sysroot? My understanding wasi-libc targets doesn't mention -preview1

Copy link
Contributor

@g0djan g0djan Jul 18, 2023

Choose a reason for hiding this comment

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

I tried to compile a simple file with rustc +stage1 -v --target=wasm32-wasi-preview1 main2.rs

fn main() {
        println!("hi");
}

And it failed with

  = note: rust-lld: error: cannot open crt1-command.o: No such file or directory
          rust-lld: error: unable to find library -lc

Also I ran the tests the same way I did here #112922 (comment)
and many more tests(almost 4k) failed due to the same error that I showed above.

Without this change I'm able to compile the same file to wasm32-wasi and only 191 test fails without this change instead of 4007 with this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I might be doing something wrong though, but could you try to run it yourself please @yoshuawuyts

Copy link
Contributor

@g0djan g0djan Jul 18, 2023

Choose a reason for hiding this comment

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

tried to fix it by removing -preview1 from here, but it doesn't fix the problem

Copy link
Contributor

Choose a reason for hiding this comment

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

tried to fix it by removing -preview1 from here, but it doesn't fix the problem

You most likely have to revert the other change in the same file (line 411) as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I did, stiil the same

Copy link
Contributor

@g0djan g0djan Jul 18, 2023

Choose a reason for hiding this comment

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

Okay, several lines above it should check for containing wasi instead of ending with it. I will send a patch

Copy link
Contributor

@g0djan g0djan Jul 18, 2023

Choose a reason for hiding this comment

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

I think I don't have permissions to update your branch @yoshuawuyts , here's the fix + merge master branch and resolving of conflicts https://github.com/rust-lang/rust/compare/master...g0djan:rust:rename-wasm32-wasi?expand=1

I also reran ui tests and now your pr doesn't break or fix any tests compared to running tests without your pr.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've spoken to the maintainers of wasi-libc, and they indeed haven't updated the target yet to include the -preview1 suffix. This is coming down the pipeline at some point in the near future, but hasn't happened yet.

So thank you for catching and fixing this; we indeed should land this change!

@Mark-Simulacrum
Copy link
Member

Apologies for the delay, I've kicked off dev-static nightly for e1e8b3f26b4661e6bcda6a18408e8fc8b5ed2d8a now.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2023
@jeffparsons
Copy link
Contributor

There's a wasi-libc PR up for renaming wasm32-wasi-threads to wasm32-wasi-preview1-threads: WebAssembly/wasi-libc#434.

I don't think it touches the wasm32-wasi target, though. Maybe best to let that PR land and then put up an equivalent PR?

@Dylan-DPC
Copy link
Member

@yoshuawuyts what's the update on this?

@alexcrichton
Copy link
Member

I've opened a new MCP with a slightly amended plan to the current tracking issue which is intended to help smooth the transition here and avoiding the need to update Rustup/Cargo with knowledge of this rename. If/when that is accepted I plan on helping out here to move this PR forward.

@alexcrichton
Copy link
Member

I've opted to (read, my memory is so bad I forgot about this, again) make a "fresh" version of this at #120468 to implement the first step of the transition plan in rust-lang/compiler-team#695

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Jan 30, 2024

Closing this in favor of #120468

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.