-
Notifications
You must be signed in to change notification settings - Fork 38
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
Remove js_of_ocaml #310
base: main
Are you sure you want to change the base?
Remove js_of_ocaml #310
Conversation
Good work on enabling Now, if you see the CI run, the This is since the version on For that, you will have to add a new version of A couple of items need to be updated in the opam file:
The CI should be green after this. |
Yep, it's the one you pointed to. |
I committed and pushed this new change but I think it still failed. Did I do something wrong? |
Line 495 of the log at https://cloud.drone.io/ocaml-bench/sandmark/1053/3/2 seems to be giving the same error message as before. |
I think run completed successfully now: https://cloud.drone.io/ocaml-bench/sandmark/1054/3/2. Great work @moazzammoriani! :) Before merge this PR, could you do a run on your local machine and report the results for the |
I suspect the error is due to CPU number here: https://github.com/ocaml-bench/sandmark/blob/main/run_config.json#L5. I'd change the We usually run it on >20 core server machines, hence 5 is a valid CPU number. Laptop/PCs usually have 4 cores and 5 isn't a valid number for it. Hope this helps. |
Unfortunately that did not fix it :( |
All right! Afraid not, I'll give this a spin tomorrow and let you know what the error is. Meanwhile, feel free to pick up some other issue if you'd like. |
I did a run on this branch locally and I think I've narrowed down what the error is; so the benchmark uses |
This PR Fixes #18.