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

Add wasm-3.0 branch to the sync'd tests here #110

Merged
merged 3 commits into from
Nov 20, 2024

Conversation

alexcrichton
Copy link
Contributor

I've tried to hack this into update-testsuite.sh but I probably made a mistake here or there. I believe this handles the removed tests in #109 though.

Update a few variables to handle that it's a branch of the `spec` repo
itself.
  spec:
    WebAssembly/spec@bd2aa85d
  memory64:
    WebAssembly/memory64@334f93f9
  custom-page-sizes:
    WebAssembly/custom-page-sizes@ba05ecdd
  wasm-3.0:
    WebAssembly/spec@90cbd509

This change was automatically generated by `update-testsuite.sh`
@alexcrichton alexcrichton requested a review from a team as a code owner November 16, 2024 05:08
@alexcrichton alexcrichton requested review from tlively and removed request for a team November 16, 2024 05:08
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Nov 16, 2024
Some minor updates in preparation for WebAssembly/testsuite#110:

* Don't limit the size of tables in the validator and instead defer such
  maximal validation to runtimes themselves.
* Parse the `(pagesize N)` syntax on "inline memories"
* Always parse table/memory limits as 64-bit integers
* Stop matching spec test suite error messages

This last point is something I've tried to hold off on doing for a long
time but as the giant `error_matches` function continues to grow and
become more unwieldy it has become less and less tenable to do this.
Overall it doesn't seem too beneficial to keep maintaining this
especially in the face of numerous proposals, so instead basically
remove it entirely.
update-testsuite.sh Outdated Show resolved Hide resolved
update-testsuite.sh Outdated Show resolved Hide resolved
@@ -80,6 +85,8 @@ merge_with_spec() {

set_upstream ${repo}

[ "${repo}" == "wasm-3.0" ] && return
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment here describing why we early return in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In commenting this I realized it was actually easier to just go ahead and handle this, so it no longer early-returns

update-testsuite.sh Outdated Show resolved Hide resolved
Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

I'm curious why the removal from #109 are not also part of this change?

github-merge-queue bot pushed a commit to bytecodealliance/wasm-tools that referenced this pull request Nov 18, 2024
* Prepare to update spec test suite

Some minor updates in preparation for WebAssembly/testsuite#110:

* Don't limit the size of tables in the validator and instead defer such
  maximal validation to runtimes themselves.
* Parse the `(pagesize N)` syntax on "inline memories"
* Always parse table/memory limits as 64-bit integers
* Stop matching spec test suite error messages

This last point is something I've tried to hold off on doing for a long
time but as the giant `error_matches` function continues to grow and
become more unwieldy it has become less and less tenable to do this.
Overall it doesn't seem too beneficial to keep maintaining this
especially in the face of numerous proposals, so instead basically
remove it entirely.

* Rename test
@alexcrichton
Copy link
Contributor Author

I think they're pseudo-folded in here perhaps? For example memory64/binary-leb128.wast moved from its current location to wasm-3.0/binary-leb128.wast in this PR. I spot checked a few other deleted files from that PR and I think this PR is basically moving them from memory64 to the wasm-3.0 directory

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Ah I see. Thanks for the explanation.

Copy link
Member

@sbc100 sbc100 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 we are approaching max-complexity for bash scripts here.. maybe we should convert this to python at some point in the future?

@alexcrichton
Copy link
Contributor Author

I felt like giving it a spin so I've pushed a rewrite from bash to python in this PR as well. I'm happy to split that out to a separate PR if preferred. I've tested it locally by ensuring that if I run it it does nothing (e.g. no updates) and then if I revert the updates made in this PR it generates the same set of updates.

One major change from before is that it no longer runs git clone or manages a git repository for each upstream proposal. I found that made the script quite slow the first time it was run when internet wasn't fast because the spec repo is relatively large. Now there's just a single git checkout and everything is fetched into that so shared history should only be fetched at most once. Additionally only a single branch is fetched which should avoid binary artifacts on other branches.

@sbc100
Copy link
Member

sbc100 commented Nov 18, 2024

I felt like giving it a spin so I've pushed a rewrite from bash to python in this PR as well. I'm happy to split that out to a separate PR if preferred. I've tested it locally by ensuring that if I run it it does nothing (e.g. no updates) and then if I revert the updates made in this PR it generates the same set of updates.

One major change from before is that it no longer runs git clone or manages a git repository for each upstream proposal. I found that made the script quite slow the first time it was run when internet wasn't fast because the spec repo is relatively large. Now there's just a single git checkout and everything is fetched into that so shared history should only be fetched at most once. Additionally only a single branch is fetched which should avoid binary artifacts on other branches.

Oh wow! Nice work. I do think it might be nice to land this functional change first, before reviewing the re-write. Can we land the version prior to the re-write?

@alexcrichton
Copy link
Contributor Author

Sounds good, rolled back that commit and I'll post it after this merges

@alexcrichton
Copy link
Contributor Author

Oh I'll interpret @sbc100 your approval as "ok for me to merge" so I'm going to merge. (sorry I forget which projects prefer others merge vs which are ok with self-merging)

@alexcrichton alexcrichton merged commit 9b4b015 into WebAssembly:main Nov 20, 2024
@alexcrichton alexcrichton deleted the wasm3-suite branch November 20, 2024 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants