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

Merge from upstream spec #262

Merged
merged 32 commits into from
Feb 22, 2023
Merged

Merge from upstream spec #262

merged 32 commits into from
Feb 22, 2023

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Feb 17, 2023

No description provided.

@aheejin aheejin requested a review from dschuff February 17, 2023 22:39
Copy link
Member

@dschuff dschuff left a comment

Choose a reason for hiding this comment

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

LGTM, but IMO you don't really even need a review on this kind of change

@aheejin aheejin force-pushed the merge_spec branch 2 times, most recently from af1d6ac to 09e3b45 Compare February 18, 2023 01:52
@aheejin
Copy link
Member Author

aheejin commented Feb 18, 2023

Currently the CI has two failures. One is 'build-core-spec' failure, which is also reproduced in the spec repo (WebAssembly/spec#1603).

The other is 'ref-interpreter' fails on binary.wast (https://github.com/WebAssembly/exception-handling/actions/runs/4208770248/jobs/7305156280, which I'm not sure why. This failure is not reproduced in my local machine. This is the reason why I'm now trying various random changes and reverting (by force-pushing) them.

(I'm now trying to revert commits in the merge to figure out which commit is causing the issue. All these reverting commits will be re-reverted.)

@aheejin
Copy link
Member Author

aheejin commented Feb 18, 2023

It looks 31cabf8 (from WebAssembly/spec#1595) is causing the 'ref-interpreter' failure (https://github.com/WebAssembly/exception-handling/actions/runs/4209147102/jobs/7305884276), but I haven't figured out why or why it's not causing the same problem in the spec repo.

@aheejin aheejin force-pushed the merge_spec branch 3 times, most recently from ef6d3e2 to a0065f1 Compare February 18, 2023 09:29
@aheejin
Copy link
Member Author

aheejin commented Feb 18, 2023

I was able to reproduce the error on my machine now. The command is

(from the interpreter dir)
$ ../test/core/run.py --wasm $PWD/wasm --js node

And the node should be v18, as 31cabf8 says. (My local node was a newer one, v19.5, which is the current LTS. The error was not reproduced on this version.) Haven't figured out why node v18 fails only on the EH repo (and not the spec repo) yet. I didn't even know we need node to run the core spec tests....

@rossberg
Copy link
Member

rossberg commented Feb 21, 2023

The test likely fails because node does not yet implement/expose exception handling. Until it does, you'll have to remove the JS=node on line 26 of .github/workflows/main.yml to deactivate running the test suite through node in CI.

I don't know why the spec build started failing, that was news to me.

rossberg and others added 6 commits February 21, 2023 12:28
JSC will have to do asynchronous compilation work during some instantiations.
To be consistent, this PR always queues a task to complete instantiation,
except through the synchronous Instance(module) API, to ensure consistency
across platforms.

This patch also cleans up the specification in various surrounding ways:
- Include notes about APIs whose use is discouraged/may be limited

Closes #741
See also webpack/webpack#6433
The section name has changed to the tag section a few years ago. This
adds the corresponding changes added in
WebAssembly#252 and
WebAssembly#256.
@Ms2ger Ms2ger merged commit 87c964f into WebAssembly:main Feb 22, 2023
@aheejin aheejin deleted the merge_spec branch February 22, 2023 19:00
@aheejin
Copy link
Member Author

aheejin commented Feb 22, 2023

@rossberg

The test likely fails because node does not yet implement/expose exception handling. Until it does, you'll have to remove the JS=node on line 26 of .github/workflows/main.yml to deactivate running the test suite through node in CI.

Node started to support EH without a flag starting v17: https://webassembly.org/roadmap/
So it's a node issue but doesn't seem to be the version issue. I'm trying to figure out what the issue is. (Node v19 doesn't have the issue anymore)

By the way I thought core spec tests are run by the reference interpreter... Are they run by node instead now? What happens if I comment out that node line? Are they run by the interpreter or not run?

@aheejin
Copy link
Member Author

aheejin commented Feb 22, 2023

@Ms2ger Sorry, but I'm asking this for the third time now. Can you please let me merge my own PRs, in the WebAssembly repos where I have write access? The ref-interpreter CI was failing and I was trying to fix it before merging. As I said in my previous comments as well, I prefer merging my PRs myself, for these reasons:

  • When I ask review for multiple reviewers, I want to wait for some more time for other reviewers to comment so I can address them.
  • There can be some CI errors I want to address before merging.
  • I want to edit my commit messages and prefer that over an automatically generated commit message, which concatenates all my individual commits in that PR.

Given that almost all people who post PRs in this repo have write access anyway, can we leave PRs to be merged by authors, maybe at least in this repo?

@rossberg
Copy link
Member

By the way I thought core spec tests are run by the reference interpreter... Are they run by node instead now? What happens if I comment out that node line? Are they run by the interpreter or not run?

They are run by both. The node run is to ensure that the JS test generation of the interpreter works correctly. If you don't set the js command then it will still convert the tests to JS but not test the result.

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.