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

Adapt JSOO rules for new --effects= option #11222

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

OlivierNicole
Copy link

This takes into account the new js_of_ocaml --effects={cps,double-translation} flag, introduced in ocsigen/js_of_ocaml#1461.

@OlivierNicole
Copy link
Author

Note: I have used failwith when a dune rule is asking to pass --effects=double-translation with a version of jsoo/wasmoo that doesn’t support it, because I don’t know what the proper way of erroring in this code path is.

@vouillon
Copy link
Member

Note: I have used failwith when a dune rule is asking to pass --effects=double-translation with a version of jsoo/wasmoo that doesn’t support it, because I don’t know what the proper way of erroring in this code path is.

I think you can just use --enable=effects in that case, and let the invocations of js_of_ocaml --effects=double-translation fail.

@OlivierNicole OlivierNicole force-pushed the jsoo-effects branch 3 times, most recently from 9501684 to 00390c3 Compare December 20, 2024 10:31
@OlivierNicole
Copy link
Author

I’m not sure to understand where the last CI failure comes from.

@OlivierNicole OlivierNicole marked this pull request as ready for review December 20, 2024 14:04
@OlivierNicole OlivierNicole force-pushed the jsoo-effects branch 3 times, most recently from 4b0b255 to 9181230 Compare January 23, 2025 15:14
@OlivierNicole
Copy link
Author

Thank you for your reviews, I think I have addressed the ones made up to now.

@hhugo
Copy link
Collaborator

hhugo commented Jan 24, 2025

I think you should add some tests that you would run manually now and that would run in the CI once jsoo.6.0 is released.

I've tested this PR manually and it doesn't quite work because the --effects flags is sometime repeated (which is treated as an error by jsoo)

@OlivierNicole
Copy link
Author

Can you share how you encountered the bug?

@hhugo
Copy link
Collaborator

hhugo commented Jan 29, 2025

I've just tried to use this PR to run test in jsoo. Same behavior in https://github.com/ocsigen/js_of_ocaml/actions/runs/13032240200/job/36353967506?pr=1821

@OlivierNicole
Copy link
Author

I think it should be fixed now.

I will look at adding tests, should they be in this PR?

@OlivierNicole OlivierNicole force-pushed the jsoo-effects branch 2 times, most recently from 7f6f913 to 2b62627 Compare January 29, 2025 16:47
@OlivierNicole OlivierNicole force-pushed the jsoo-effects branch 2 times, most recently from 4e7b45e to 96b7771 Compare February 12, 2025 16:07
OlivierNicole and others added 14 commits February 13, 2025 14:56
Signed-off-by: Olivier Nicole <[email protected]>
Signed-off-by: Olivier Nicole <[email protected]>
Signed-off-by: Olivier Nicole <[email protected]>
Signed-off-by: Olivier Nicole <[email protected]>
Signed-off-by: Olivier Nicole <[email protected]>
Signed-off-by: Olivier Nicole <[email protected]>
Co-authored-by: Jérome Vouillon <[email protected]>
Signed-off-by: Olivier Nicole <[email protected]>
Signed-off-by: Olivier Nicole <[email protected]>
Signed-off-by: Olivier Nicole <[email protected]>
Signed-off-by: Olivier Nicole <[email protected]>
Signed-off-by: Olivier Nicole <[email protected]>
Signed-off-by: Olivier Nicole <[email protected]>
Signed-off-by: Olivier Nicole <[email protected]>
@OlivierNicole
Copy link
Author

I have added a test but for it to work, it would require to use JSOO 6.0.1 in CI.

The other failures seem unrelated to this PR.

@OlivierNicole
Copy link
Author

I have added a test but for it to work, it would require to use JSOO 6.0.1 in CI.

Oh in fact, it’s already the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants