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.

Sign up for GitHub

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

Jump to bottom

[Wasm64] Run core tests under wasm64 as well as wasm64l #17803

Merged
merged 1 commit into from Sep 13, 2022
Merged

[Wasm64] Run core tests under wasm64 as well as wasm64l #17803

merged 1 commit into from Sep 13, 2022

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Sep 6, 2022

No description provided.

@sbc100 sbc100 force-pushed the wasm64 branch 3 times, most recently from 1c6879f to 9536581 Compare September 6, 2022 14:46
@sbc100 sbc100 enabled auto-merge (squash) September 6, 2022 18:51
@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 6, 2022

Because wasm64 tests only run under d8 this requires some fixes of things that were just related to being able to run stuff under d8.

kripken
kripken reviewed Sep 6, 2022
View reviewed changes
.circleci/config.yml Show resolved Hide resolved
test/test_core.py Outdated Show resolved Hide resolved
@dschuff
Copy link
Member

dschuff commented Sep 8, 2022

Does wasm64 not run under Chrome even with a flag? I thought it was possible to pass arbitrary JS flags through to V8?

@sbc100 sbc100 force-pushed the wasm64 branch 2 times, most recently from 4e299a2 to d59cd4c Compare September 9, 2022 12:51
@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 9, 2022

Does wasm64 not run under Chrome even with a flag? I thought it was possible to pass arbitrary JS flags through to V8?

It most likely does but we want but it would be a shame to have to more all the wasm64 testing into browser tests.. and we don't currently run the core tests in the browser (nor do we want to start doing that).

@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 9, 2022

I think we should land this since it fixes a bunch of issues, especially in the test code. We can debate removing wasm64l seperately/later. This only adds 2 minutes to an already fast builder as it stands.

dschuff
dschuff reviewed Sep 9, 2022
View reviewed changes
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.

agreed that we can go ahead and get this tested ASAP, independently of wasm64l.

test/test_core.py Outdated Show resolved Hide resolved
test/test_core.py Show resolved Hide resolved
@dschuff
Copy link
Member

dschuff commented Sep 9, 2022

BTW the "Current" version of node (18.9.0) says it has support for the --experimental-wasm-memory64 flag, so that might be an option too.

@sbc100 sbc100 merged commit f32e74b into main Sep 13, 2022
@sbc100 sbc100 deleted the wasm64 branch September 13, 2022 17:13
@tiran
Copy link
Contributor

tiran commented Sep 13, 2022

BTW the "Current" version of node (18.9.0) says it has support for the --experimental-wasm-memory64 flag, so that might be an option too.

I have been using Node 16.14.0 for wasm64 with great success.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kripken kripken kripken approved these changes

@dschuff dschuff Awaiting requested review from dschuff

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sbc100 @dschuff @tiran @kripken