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

add ocaml-variants.5.1.1+flambda2 #25928

Closed
wants to merge 9 commits into from
Closed

add ocaml-variants.5.1.1+flambda2 #25928

wants to merge 9 commits into from

Conversation

avsm
Copy link
Member

@avsm avsm commented May 23, 2024

This introduces the flambda2 in-development variant, along with the various base packages required to compile it. These are secondary packages for dune/menhir (required to bootstrap the development version of the forked compiler), and also a new base-flambda2 package to

The only variation to package selection should be that the new ocaml-secondary-compiler could be selected by existing ocamlfind-secondary packages, but it should be fine as it is just a newer version of the bootstrap OCaml used to build the ocamlfind binaries.

Respin of #25757, with additional testing over at https://github.com/avsm/fl2-opam-repo. Note that many ecosystem packages currently do not compile with this compiler variant, notably ocamlbuild onwards. But it's very useful indeed to have the secondary packages to make the compiler variant available to start adapting those downstream dependencies.

avsm and others added 5 commits May 23, 2024 14:47
This introduces the flambda2 in-development variant, along with
the various base packages required to compile it. These are secondary
packages for dune/menhir (required to bootstrap the development version
of the forked compiler), and also a new base-flambda2 package to

The only variation to package selection should be that the new
ocaml-secondary-compiler could be selected by existing ocamlfind-secondary
packages, but it should be fine as it is just a newer version of
the bootstrap OCaml used to build the ocamlfind binaries.
Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

Minor suggestions inline, but I think it's worth doing two things which are in the commits in https://github.com/dra27/opam-repository/commits/js-comp:

  • The use of build-env in dune-secondary.3.8.1 implies that opam 2.1.5 or later is needed, but I personally think that's fine - so the first commit simplifies menhir-secondary and the compiler package to use build-env instead of sh
  • The original idea behind ocaml-secondary-compiler is that it is an entire compiler - it seems neater to install menhir-secondary and dune-secondary to the same location, as it then means that the compiler package just needs to add the one bin directory.

As a bonus, there's a commit using 4.14.2 instead of of 4.14.1 🙂

packages/dune-secondary/dune-secondary.3.8.1/opam Outdated Show resolved Hide resolved
packages/menhir-secondary/menhir-secondary.20210419/opam Outdated Show resolved Hide resolved
packages/menhir-secondary/menhir-secondary.20210419/opam Outdated Show resolved Hide resolved
"jbuilder" {= "transition"}
]
dev-repo: "git+https://github.com/ocaml/dune.git"
build-env: [PATH += "%{ocaml-secondary-compiler:share}%/bin"]
Copy link
Member

@dra27 dra27 May 23, 2024

Choose a reason for hiding this comment

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

Unfortunately, this requires ocaml/opam#5352, which restricts it to opam 2.1.5+

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 9871990 to support both versions. I still have a number of machines on (e.g.) Ubuntu 22.04 which still have opam 2.1.2 as their pre-bundled version. It's uglier here to duplicate the build commands, but we can just delete the extra lines when opam < 2.1.5 rotate out of circulation in a few years.

Copy link
Member

Choose a reason for hiding this comment

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

Darn, I'm really sorry - it is indeed true that ocaml/opam#5352 is necessary for the variable to be set correctly, but then the issue I've identified in ocaml/opam#6051 bites... %{ocaml-secondary-compiler:share}%/bin appears after the bin directory, so you still get the 4.07 compiler in the switch rather than the secondary one.

Alas, it would appear that for now it has to be done with the ugly "sh" "-exc" version, but I've opened that issue/draft-PR to track improving this.

"base-threads" {post}
"base-flambda2" {post}
"dune-secondary" {="3.8.1"}
"menhir-secondary"
Copy link
Member

Choose a reason for hiding this comment

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

menhir-secondary (as dune-secondary) is quite old - is there a reason it's not therefore constrained here? (curiosity more than anything else)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm deferring to downstream here, as those are the versions they are using for their bootstrap.

Choose a reason for hiding this comment

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

I think menhir versions need to stay as-is, at least for the moment.

"base-bigarray" {post}
"base-threads" {post}
"base-flambda2" {post}
"dune-secondary" {="3.8.1"}
Copy link
Member

Choose a reason for hiding this comment

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

I'm presuming it really has to be 3.8.1 and nothing more recent?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm deferring to downstream here, as those are the versions they are using for their bootstrap.

Choose a reason for hiding this comment

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

I think actually it's ok to use a more recent dune now.

@avsm
Copy link
Member Author

avsm commented Jun 17, 2024

Tested on both opam 2.1.6 and opam 2.1.2 and confirmed working, and constrained to linux/x86_64 only as other architecture support isn't present yet. @dra27 might I trouble you to re-review?

@avsm
Copy link
Member Author

avsm commented Jun 18, 2024

I don't understand why dune-secondary is failing with a base ocaml of <=4.08

#=== ERROR while compiling dune-secondary.3.8.1 ===============================#
# context              2.2.0~beta3~dev | linux/x86_64 | ocaml-base-compiler.4.07.1 | file:///home/opam/opam-repository
# path                 ~/.opam/4.07/.opam-switch/build/dune-secondary.3.8.1
# command              ~/.opam/opam-init/hooks/sandbox.sh build ocaml boot/bootstrap.ml -j 31
# exit-code            2
# env-file             ~/.opam/log/dune-secondary-7-157050.env
# output-file          ~/.opam/log/dune-secondary-7-157050.out
### output ###
# ocamlfind -toolchain secondary ocamlc 2>.duneboot.ocamlfind-output
# sh: 1: ocamlfind: not found
# 
# The ocamlfind's secondary toolchain does not seem to be correctly
# installed.
# Dune requires OCaml 4.08 or later to compile.
# Please either upgrade your compile or configure a secondary OCaml compiler
# (in opam, this can be done by installing the ocamlfind-secondary package).

This doesnt trigger in the usual case (of installing a 5.1.1+flambda2) but it "should" work. Is there a missing ocamlfind-secondary in the dune-secondary package for ocaml<4.08? Advice from @dra27 welcome as the secondary expert :-)

@mshinwell
Copy link

A few minor points:

  • I'm not sure whether the flambda-backend versioning scheme should be reflected in the OPAM versions in some way. That would require building from the tagged releases e.g. 5.1.1minus-17. It seems also good to have a package that installs from tip of main, although I'm unsure how OPAM copes with this when the corresponding revision number changes, as it does very frequently.
  • Maybe the version should be "5.1.1minus" not "5.1.1" at the moment? The "minus" indicates that this isn't quite OCaml 5; in particular domains and effects are not supported yet, although will be in the forthcoming months.
  • Maybe the name should mention "flambda-backend" not "flambda2"? Flambda 2 is only one part of the myriad of changes that distinguishes this from upstream OCaml (e.g. local allocations, Cfg-based backend). (Flambda 2 is also the only supported middle end; flambda-backend has neither Closure nor Flambda 1.)

@mseri
Copy link
Member

mseri commented Jul 10, 2024

What is the status of this PR? If there is still work to do, I would suggest closing it since it exhaust the CI each time it runs (it happened recently when CI changes had to be reverted)

@avsm avsm closed this Jul 20, 2024
@avsm
Copy link
Member Author

avsm commented Jul 20, 2024

I'll reopen it when it's ready again!

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.

5 participants