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

update WASI submodule #3025

Merged
merged 4 commits into from
Jun 24, 2021
Merged

Conversation

cratelyn
Copy link
Member

@cratelyn cratelyn commented Jun 23, 2021

This PR updates our assorted witx dependencies to 0.9.1, which includes an important bug fix. See WebAssembly/WASI#434 for more information.

@alexcrichton
Copy link
Member

Thanks! FWIW the location of the WASI submodule is pretty finnicky especially when it comes to publishing to crates.io. For the easiest time updating I'd recommend updating it in-place if possible

@github-actions github-actions bot added the wasi Issues pertaining to WASI label Jun 23, 2021
@github-actions
Copy link

Subscribe to Label Action

cc @kubkon

This issue or pull request has been labeled: "wasi"

Thus the following users have been cc'd because of the following labels:

  • kubkon: wasi

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@cratelyn
Copy link
Member Author

Thanks! FWIW the location of the WASI submodule is pretty finnicky especially when it comes to publishing to crates.io. For the easiest time updating I'd recommend updating it in-place if possible

Thanks @alexcrichton! I'll give that another go, moving the submodule was motivated by errors I was seeing that looked like a variation of rust-lang/cargo#6745. I wasn't able to exclude that directory from the root workspace, so commands like cargo build were failing due to multiple workspace roots being found.

@cratelyn
Copy link
Member Author

cratelyn commented Jun 23, 2021

error: multiple workspace roots found in the same workspace:
  /home/katelyn/fsly/wasmtime/crates/wasi-common/WASI/tools/witx
  /home/katelyn/fsly/wasmtime

For reference, this is the error I am seeing after updating the WASI submodule. I believe it's because of the witx-cli changes in WebAssembly/WASI#398 playing poorly with structure of this project.

As alluded in my comment above, adding an exclude = ["crates/wasi-common/WASI/tools/witx"] to the root manifest doesn't end up working, because "crates/wasi-common" is a member of the workspace.

@alexcrichton
Copy link
Member

Oh dear... I think there's probably three ways to fix this:

  1. Remove the workspace changes to the WASI repository, enabling integration here the same as before.
  2. Fix the issues after moving this vendor dir around (I don't know off-hand how hard this would be)
  3. Depend on the crates.io version of witx, not a git submodule. I think that this would require doing the same steps from (2), but we could just avoid a git submodule.

The easiest is probably the first one here, and the best is probably the 3rd, but that requires changes of how we integrate the WASI spec updates into this repository, since I think we rely on them existing in the wasi-common crate but now they'd be stored in the witx crate itself (I think?) published to crates.io

@alexcrichton
Copy link
Member

Hm actually scratch that I don't think the witx crate as published to crates.io contains the wasi snapshot witx files. Sorry for this mess :(

I'd probably recommend just hacking around this by removing the [workspace] in the witx crate for now? Ideally we'll fix this in the long term by separating the tooling and the *.witx files since we want tooling as a crates.io dep and the *.witx files as a submodule.

katelyn martin added 2 commits June 23, 2021 17:25
This updates the WASI submodule, pulling in changes to the witx crate,
now that there is a 0.9.1 version including some bug fixes. See
WebAssembly/WASI#434 for more information.
@cratelyn
Copy link
Member Author

Run rustc scripts/publish.rs
thread 'main' panicked at 'failed to find "witx-cli" in whitelist or blacklist', scripts/publish.rs:135:13

This shouldn't be too hard to fix, but I see some special logic around witx and that'll have to wait until tomorrow morning for me.

@cratelyn
Copy link
Member Author

Oh I'm going to mark this as "ready for review" since we're kinda already in the process of reviewing this work. 👍

@cratelyn cratelyn marked this pull request as ready for review June 23, 2021 21:42
@cratelyn
Copy link
Member Author

cratelyn commented Jun 24, 2021

error: current package believes it's in a workspace when it's not:
current:   /home/runner/work/wasmtime/wasmtime/crates/wasi-common/WASI/tools/witx-cli/Cargo.toml
workspace: /home/runner/work/wasmtime/wasmtime/Cargo.toml

this may be fixable by adding `crates/wasi-common/WASI/tools/witx-cli` to the `workspace.members` array of the manifest located at: /home/runner/work/wasmtime/wasmtime/Cargo.toml
Alternatively, to keep it out of the workspace, add the package to the `workspace.exclude` array, or add an empty `[workspace]` table to the package's manifest.

Oh no 😨 It seems it's our old friend again. I'm trying to find a way around this. I've tried...

1️⃣ Add witx-cli to root workspace.members

This fails, with an error that looks like:

error: no matching package named `diff` found
location searched: registry `https://github.com/rust-lang/crates.io-index`
perhaps you meant: ff, der, half, ...
required by package `witx-cli v0.9.1 (/home/katelyn/fsly/wasmtime/crates/wasi-common/WASI/tools/witx-cli)`

Not entirely sure why this happens, honestly? But it seems... wrong.

2️⃣ Add witx-cli to root workspace.exclude

This doesn't change the error, AIUI this is the cargo issue I linked in a comment above.

3️⃣ Make crates/wasi-common a workspace

This one feels like a non-starter, as we're instantly in a state of multiple workspace roots.

This commit removes some items from the root manifest's workspace
members array, and adds `witx-cli` to the root `workspace.exclude`
array.

The motivation for this stems from a cargo bug described in
rust-lang/cargo#6745: `workspace.exclude` does not work if it is nested
under a `workspace.members` path.

See WebAssembly/WASI#438 for the underlying change to the WASI submodule
which reorganized the `witx-cli` crate, and WebAssembly/WASI#398 for the
original PR introducing `witx-cli`.

See [this
comment](bytecodealliance#3025 (comment))
for more details about the compilation errors, and failed alternative
approaches that necessitated this change.

N.B. This is not a functional change, these crates are still implicitly
workspace members as transitive dependencies, but this will allow us to
side-step the aforementioned cargo bug.

Co-Authored-By: Alex Crichton <alex@alexcrichton.com>
@alexcrichton alexcrichton merged commit ab53612 into bytecodealliance:main Jun 24, 2021
@cratelyn cratelyn deleted the katie/update-witx branch June 24, 2021 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants