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

[ci] Add 32bit 5.2.0 job #1700

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ejgallego
Copy link
Contributor

@ejgallego ejgallego commented Oct 1, 2024

Caveats:

  • The OCaml switch in this case byte-only, so things are slow, as jsoo itself won't have a native mode

  • JS ppx libs 0.17 don't really support 32bit builds, this will likely become a problem in the future (if not now actually, for example ppx_inline_test.0.17.0 fails to build)

  • jsCoq/coq-lsp works in this setup, which is IMO a great "data point" as to whether this setup is OK

p.s: Coq stops working in 5.2.0 if I pass --enable=effects tho, the error is Reason: Uncaught RangeError: Maximum call stack size exceeded, is that worth reporting?

@ejgallego ejgallego force-pushed the ci_52_32bit branch 2 times, most recently from 7229f1b to 72f959d Compare October 1, 2024 15:37
@ejgallego
Copy link
Contributor Author

Indeed, even if I did check git grep ppx_inline_test in JSOO sources, the package got pulled transitively.

So we either:

  • drop this PR
  • fix time_now.0.17.0 to build in 32bit (which may not be possible due to upstream policy)
  • softer testing reqs for 5.2.0 + 32 bits

Any other idea?

@hhugo
Copy link
Member

hhugo commented Oct 1, 2024

No need to run the tests. Just build/install like we do for older ocaml version

@ejgallego
Copy link
Contributor Author

ejgallego commented Oct 1, 2024

@hhugo should we still do dune build @all --profile using-effects in the 5.2.0+32bit job?

As of now the @runtest part fails indeed.

Caveats:

- The OCaml switch in this case byte-only, so things are slow, as jsoo
  itself won't have a native mode

- JS ppx libs 0.17 don't really support 32bit builds, this will likely
  become a problem in the future (if not now actually, for example
  `ppx_inline_test.0.17.0` fails to build)

- jsCoq/coq-lsp works in this setup, which is IMO a great "data point"
  as to whether this setup is OK
@@ -82,7 +88,7 @@ jobs:
# getting much better, but no luck yet, c.f:
# https://github.com/ocaml/opam-repository/pull/26626
- name: Install apt 32-bit dependencies
if: matrix.ocaml-compiler == 'ocaml-variants.4.14.2+options,ocaml-option-32bit'
if: matrix.ocaml-compiler == 'ocaml-variants.4.14.2+options,ocaml-option-32bit' || matrix.ocaml-compiler == 'ocaml-variants.5.2.0+options,ocaml-option-32bit'
Copy link
Member

Choose a reason for hiding this comment

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

Should should be able to use contains( matrix.ocaml-compiler, "ocaml-option-32bit")

@hhugo
Copy link
Member

hhugo commented Oct 1, 2024

@hhugo should we still do dune build @all --profile using-effects in the 5.2.0+32bit job?

As of now the @runtest part fails indeed.

@hhugo should we still do dune build @all --profile using-effects in the 5.2.0+32bit job?

As of now the @runtest part fails indeed.

Yes, we could. You could change the ci to respect skip test when skip-effect is false

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