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

Correct the bytecode architectures for OCaml 5.1 and OCaml 5.2 (trunk) #24075

Merged
merged 2 commits into from
Jul 11, 2023

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Jul 6, 2023

This PR fixes two small issues:

  • The 5.1.0 beta does not correctly pull in the ocaml-option-bytecode-only package for ppc64
  • The 5.2 (trunk) package still has the 5.0 native architecture restrictions, when it no longer needs any

I've extensively tested this (hopefully!) on amd64, ppc64 and s390x. The important details:

  • This PR will cause 5.0 bytecode-only switches to rebuild but it does not affect amd64 and arm64 (which will only see a largely no-op rebuild of the ocaml-options-vanilla package)
  • This PR does cause all 5.1 switches to rebuild because of the change to the beta package itself (unless you're using opam 2.2 😇)
  • Existing 5.2 (i.e. trunk switches) on ppc64, riscv or s390x will be unable to upgrade without manual intervention unless opam 2.2 is being used.

@dra27
Copy link
Member Author

dra27 commented Jul 6, 2023

This is a draft for two reasons:

  • There are other ocaml-option-* packages which need to be updated for the restored architectures in 5.1 and 5.2
  • I think I made a mistake when discussing this with @kit-ty-kate months ago on having ocaml-option-bytecode-only vs base-bytecode-only. Unless I've forgotten the full detail of my counterargument, I think that we were possibly both correct and this should be revised further, but I need to do some more testing.

@dra27 dra27 marked this pull request as ready for review July 7, 2023 11:11
dra27 added 2 commits July 7, 2023 12:17
ocaml-option-bytecode-only pulled in only for ppc64.

Technically, s390x wasn't available in native code for alpha1.
@dra27
Copy link
Member Author

dra27 commented Jul 7, 2023

OK, I've changed the scope of this slightly, in order to allow the two bugs to be fixed more quickly. This revised version now only affects the 5.1 and 5.2 packages (i.e. it's clearly the case that released switches are unaffected). It's also a good deal easier to review as a result 🙂 I have again tested these on s390x and ppc64

  • The bytecode-only architectures correctly pull in ocaml-option-bytecode-only (i.e. ppc64 on 5.1 is now re-identified as being bytecode-only) and, importantly, that package cannot be removed.
  • For ppc64, s390x and riscv, the 5.2 trunk package now builds the native compiler

The critical fix is that if ocaml-option-bytecode-only is pulled in (i.e. for ppc64 before 5.2 and s390x on 5.1.0~alpha1), then it cannot be removed which means that conflicts: "ocaml-option-bytecode-only" works properly. The part of the fix I've removed is that ocaml-options-vanilla is not working correctly for the 5.2 trunk package (entirely non-urgent, therefore) or for riscv and s390x on 5.1 (relatively non-urgent). The issue is that at present in those configurations it is possible to install ocaml-option-bytecode-only, which shouldn't be permitted (adding ocaml-options-vanilla to the switch invariant is supposed to force the compiler at its default configuration). This is a much lesser concern, as there are plenty of packages which conflict ocaml-option-bytecode-only and no non-compiler packages which depend on it.

Fixing the slight mess with ocaml-options-vanilla will be both disruptive to switches using released compilers and is more complicated to review. I propose therefore to fix that alongside the Windows compilers in the next few weeks.

@avsm
Copy link
Member

avsm commented Jul 7, 2023

This looks right to me (and is surprisingly complex!). There are also a number of 32-bit-related options in the configure step which look redundant to me now, but can be fixed in a future PR. Is the idea to get this correct in opam-repository, and then submit a fix to ocaml/ocaml for the trunk metadata to be correct, @dra27? That would help @Octachron with release generation.

@dra27
Copy link
Member Author

dra27 commented Jul 7, 2023

I confess I hadn't considered the ocaml-variants.opam file in ocaml/ocaml itself (it's kinda still on the "eventually" spike for getting pinning compiler branches properly sorted... but on the basis that I originally demo'd that in early 2021, it'll probably land sometime next year 😂).

It's not related to the release process, though - I'm fairly sure @Octachron uses the previous packages in opam-repository as the starting point for the following release.

There are also a number of 32-bit-related options in the configure step which look redundant to me now

Yes, indeed! Now that 5.1 has confirmed their removal, those really ought to be simplified. I've added to the list of bits to check along with the Windows parts.

Copy link
Contributor

@jmid jmid left a comment

Choose a reason for hiding this comment

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

The constraints LGTM, reflecting that

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

Successfully merging this pull request may close these issues.

3 participants