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

Try a new layout for compiler flags for OCaml 4.12.0 (and add 4.12.0~alpha1) #17541

Merged
merged 22 commits into from
Nov 16, 2020

Conversation

AltGr
Copy link
Member

@AltGr AltGr commented Oct 30, 2020

This PR is a continuation of #16753 and the general attempt to reduce the number of ocaml-variants packages in the repository.

It's proposed to test this layout with the 4.12 development releases of OCaml - if successful, there can be further discussion for updating 4.11 and earlier to use the same idea.

What you can now do:
opam 2.1

  • opam switch create alpha-testing 4.12.0~alpha1to get the normal compiler release.
  • opam switch create alpha-testing-flambda ocaml-variants.4.12.0~alpha1+options ocaml-option-flambda to get alpha1 with flambda enabled.

opam 2.0

  • [opam repo add beta git+https://github.com/ocaml/ocaml-beta-repository.git]
  • opam switch create alpha-testing 4.12.0~alpha1 --repos=default,beta to get the normal compiler release (i.e. you need the beta repository enabled, as in previous releases)
  • opam switch create alpha-testing-flambda --packages=ocaml-variants.4.12.0~alpha1+options,ocaml-option-flambda --repos=default,beta

How it works:

  • ocaml-base-compiler is unchanged and, as today, compiles and installs OCaml with default options
  • The ocaml-variants.4.12.0+trunk package and the new ocaml-variants.4.12.0~alpha1+options packages both allow compiler options to be customised by also installing ocaml-options- packages:
    • ocaml-option-32bit - 32bit build
    • ocaml-option-afl - enable AFL support
    • ocaml-option-bytecode-only - build bytecode compiler only
    • ocaml-option-default-unsafe-string - enable unsafe-string by default
    • ocaml-option-flambda - enable flambda
    • ocaml-option-fp - enable frame pointers
    • ocaml-option-musl - use musl instead of libc
    • ocaml-option-nnp - enable no-naked-pointers mode
    • ocaml-option-no-flat-float-array - disable flat float arrays in the runtime
    • ocaml-option-spacetime (obviously, for 4.12 this option is not available)
    • ocaml-option-static - no dynamic linking
  • In particular, this allows options to be combined: for example installing ocaml-options-32bit and ocaml-options-flambda enables a 32-bit flambda compiler.
  • The package ocaml-options-vanilla can be used to make ocaml-variants.4.12.0~alpha1+options or ocaml-variants.4.12.0+trunk behave like ocaml-base-compiler. In particular if your base packages (or opam 2.1 switch invariant) include ocaml-options-vanilla then you can be sure that the compiler will not be altered if you install a package which depends on an option (for example, a package which depends on ocaml-options-flambda will not cause a switch to recompile).
  • The development variants for 4.12 have been removed, but no other variants have been at this stage
  • OCaml 4.12 numbers alpha, beta and rc releases using ~name instead of +name. Support for this was added but intentionally not used in 4.11. ocaml-config needs a small update to support this. In order to prevent lots of switches needing to recompile, this is done in ocaml-config.2 which is only available for 4.12.0 onwards.

Instead of many variants, the compiler is now `ocaml-variants.4.12.0~alpha1`,
and the variants are selected using the different `ocaml-options-*` packages.

Use `ocaml-options-vanilla` if you want to ensure no special options will be
selected, even if some packages depend on them.
@dra27 dra27 marked this pull request as draft October 30, 2020 17:12
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.

Preliminary look through - but thanks for whipping this one up so quickly! ocaml-base-compiler.4.12.0~alpha1 is missing and we don't (yet) have the full suite of ocaml-options-only- packages.

packages/ocaml/ocaml.4.12.0/opam Outdated Show resolved Hide resolved
packages/ocaml-variants/ocaml-variants.4.12.0+trunk/opam Outdated Show resolved Hide resolved
@camelus
Copy link
Contributor

camelus commented Oct 30, 2020

Commit: 7cc7d51

As you wish, master!

🌤️ opam-lint warnings 7cc7d51
  • ocaml-option-32bit.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-option-afl.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-option-bytecode-only.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-option-default-unsafe-string.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-option-flambda.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-option-fp.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-option-musl.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-option-nnp.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-option-no-flat-float-array.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-option-spacetime.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-option-static.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-options-only-afl.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-options-only-flambda-fp.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-options-only-flambda.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-options-only-fp.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-options-only-nnp.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-options-only-no-flat-float-array.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-options-vanilla.1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
  • ocaml-variants.4.12.0+trunk has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • ocaml.4.12.0 has some warnings:

    • warning 56: It is discouraged for non-compiler packages to use 'setenv:'
  • These packages passed lint tests: ocaml-base-compiler.4.12.0~alpha1, ocaml-config.2, ocaml-variants.4.12.0~alpha1+options


🌤️ Installability check (+14)
  • new installable packages (20): ocaml-base-compiler.4.12.0~alpha1 ocaml-config.2 ocaml-option-32bit.1 ocaml-option-afl.1 ocaml-option-bytecode-only.1 ocaml-option-default-unsafe-string.1 ocaml-option-flambda.1 ocaml-option-fp.1 ocaml-option-musl.1 ocaml-option-nnp.1 ocaml-option-no-flat-float-array.1 ocaml-option-static.1 ocaml-options-only-afl.1 ocaml-options-only-flambda.1 ocaml-options-only-flambda-fp.1 ocaml-options-only-fp.1 ocaml-options-only-nnp.1 ocaml-options-only-no-flat-float-array.1 ocaml-options-vanilla.1 ocaml-variants.4.12.0~alpha1+options
  • new broken packages (1): ocaml-option-spacetime.1
  • removed installable packages (6): ocaml-variants.4.12.0+trunk+afl ocaml-variants.4.12.0+trunk+flambda ocaml-variants.4.12.0+trunk+fp ocaml-variants.4.12.0+trunk+fp+flambda ocaml-variants.4.12.0+trunk+nnp ocaml-variants.4.12.0+trunk+no-flat-float-array

🌤️ 10 ignored non-opam files:
  • packages/ocaml-base-compiler/ocaml-base-compiler.4.12.0~alpha1/files/ocaml-base-compiler.install
  • packages/ocaml-config/ocaml-config.2/files/gen_ocaml_config.ml.in
  • packages/ocaml-config/ocaml-config.2/files/ocaml-config.install
  • packages/ocaml-variants/ocaml-variants.4.12.0+trunk+afl/opam
  • packages/ocaml-variants/ocaml-variants.4.12.0+trunk+flambda/opam
  • packages/ocaml-variants/ocaml-variants.4.12.0+trunk+fp+flambda/opam
  • packages/ocaml-variants/ocaml-variants.4.12.0+trunk+fp/opam
  • packages/ocaml-variants/ocaml-variants.4.12.0+trunk+nnp/opam
  • packages/ocaml-variants/ocaml-variants.4.12.0+trunk+no-flat-float-array/opam
  • packages/ocaml-variants/ocaml-variants.4.12.0~alpha1+options/files/ocaml-variants.install

@AltGr
Copy link
Member Author

AltGr commented Nov 2, 2020

Fixed all the trivial issues; not sure yet what to do with ocaml-base-compiler

The biggest use I think could be through the variables defined by the ocaml wrapper package (using scripts in ocaml-config), i.e. ocaml:compiler. It's defined at the moment as:

compiler: "%{ocaml-system:installed?system:}%%{ocaml-base-compiler:version}%%{ocaml-variants:version}%"

of course, this should be fixed if ocaml-base-compiler can be installed together with ocaml-variants (although if the former is not in the dependencies, the variable should remain undefined here).
Rebuilding the same string could be done artificially through stg like

"%{ocaml-variants:version}%%{ocaml-options-foo:installed?+foo:}%%{ocaml-options-bar:installed?+bar:}%..."

Another question: do you think we need only-* packages for all the options ??

@dra27
Copy link
Member

dra27 commented Nov 2, 2020

For now, I think we should keep ocaml-base-compiler only to keep the change strictly limited to ocaml-variants - I completely agree that it becomes less or even un- necessary.

For the only packages, once you have one, I'm not sure I can justify why you don't have the others! Which one's would you exclude from wanting to combine? 🙂

@AltGr
Copy link
Member Author

AltGr commented Nov 2, 2020

I agree on keeping ocaml-variants... I was only wondering if it was better to have it as a duplicate definition (more conservative, since it means this path is not changed at all from before), or as a virtual package (forcing ocaml-options-vanilla), in which case it would still be accessible to the user, but move outside of the deps of ocaml.

About only packages, I was assuming that they were only useful for potential "release" or "production" switches where you want to make certain your compiler doesn't get polluted by unwanted options. I wouldn't see the point for afl, for example (but yes, e.g. static would make perfect sense here). Dunno, just trying to not multiplicate packages too much at once ;)

@Octachron
Copy link
Member

Just to be sure, the shortest command line to install the vanilla compiler under the new layout will be

opam switch create alpha-testing --packages=ocaml-base-compiler.4.12.0~alpha1 --repositories=default,beta

?

@AltGr
Copy link
Member Author

AltGr commented Nov 2, 2020

@Octachron you can do shorter :)

  • --packages shouldn't be needed
  • adding the beta repo should no longer be needed with 2.1

So it could be reduced to

opam switch create alpha-testing ocaml-base-compiler.4.12.0~alpha1

To go the "newer way", that could be instead:

opam switch create alpha-testing 4.12.0~alpha1 ocaml-options-vanilla

(1 - you will need --packages in this case for opam 2.0 IIRC; 2 - you can actually skip options-vanilla, it is only useful on 2.1 if you want to make certain no compiler flags can be set later on in this switch)

@dra27 one issue I could see is that if we keep ocaml-variants.4.12.0 and ocaml-base-compiler.4.12.0, for the tweaky underspecified command opam sw cr 4.12.0, the automatic invariant will become a disjunction of the two and the solver might choose base-compiler at the moment of installation. I think this will not really be an issue though, since, due to the conflicts, if you then require an option the solver will safely switch to ocaml-variants, but worth noting.)

The package doesn't exist yet though... alpha/betas used to be under `variants`,
do we want to change that ?
@dra27
Copy link
Member

dra27 commented Nov 3, 2020

For the announcement, I suggest we stick with the opam-2.0 compatible version - it should be the case by the time OCaml 4.13 is branched that 2.1 is out, so we could for that announcement have the shorter 2.1 syntax and then have the 2.0-compatible version as a footnote, but at this stage I think it's more confusing!

@dra27
Copy link
Member

dra27 commented Nov 3, 2020

For the ocaml-options-only packages, the problem is that as soon as someone wants one that isn't there, it's a nuisance! I agree that this PR can readily just have the flambda one, to correspond with upgrading the existing variants packages, but I think we should shortly after have it such ocaml-options-only-bar-foo-widget definitely exists for any combination of the options (in alphabetical order)

@dra27
Copy link
Member

dra27 commented Nov 3, 2020

This won't matter until release time, but we also need to remember that ocaml-system must depend on ocaml-options-vanilla

…base-compiler

esp. for opam 2.0 and for the 'opam switch create NAME VERSION' shortcut
@AltGr
Copy link
Member Author

AltGr commented Nov 5, 2020

This won't matter until release time, but we also need to remember that ocaml-system must depend on ocaml-options-vanilla

Not strictly required, as since all the options have a post-depend on the proper ocaml-variants, we still get the guarantee that they can't be used together with ocaml-system. But yes, that may make things clearer.

@Octachron
Copy link
Member

Octachron commented Nov 5, 2020

The ocaml-config package also needs to be updated to make gen_ocaml_config.ml.in aware of ~, otherwise the installation fails with a version mismatch.

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.

Spotted a stray ocaml-compiler

@@ -0,0 +1 @@
share_root: ["config.cache" {"ocaml/config.cache"}]
Copy link
Member

Choose a reason for hiding this comment

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

This file needs renaming to ocaml-variants.install

src: "https://github.com/ocaml/ocaml/archive/4.12.0-alpha1.tar.gz"
checksum: "sha256=bee59cb94067410d02f0bc4e7e47e3e878689aabf61e6d2f0cb4316f8563e55d"
}
extra-files: ["ocaml-compiler.install" "md5=3e969b841df1f51ca448e6e6295cb451"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
extra-files: ["ocaml-compiler.install" "md5=3e969b841df1f51ca448e6e6295cb451"]
extra-files: ["ocaml-variants.install" "md5=3e969b841df1f51ca448e6e6295cb451"]

@Octachron
Copy link
Member

Another point, installing compiler with

opam switch create 4.12.0~alpha1+flambda --packages=ocaml-variants.4.12.0~alpha1+options,ocaml-options-flambda --repo=beta

yields a warning with opam 2.07

Packages { ocaml-options-flambda.1 } don't have the 'compiler' flag set. Are you sure you want to set them as the compiler base for this switch? [y/N]

Should the option packages set this compiler flag for compatibility's sake?

Also, I have a patch for ocaml-config, would it help if I sent it as a subpr?

@dra27
Copy link
Member

dra27 commented Nov 6, 2020

Oops, sorry - I'd had a discussion with @AltGr offline and not reported the relevant bits back here. I agree that ocaml-options-* packages need the compiler flag to get rid of that message. That flag means they won't warn about being installed in the base (and won't warn if not installed in the base) of the switch.

I'd also proposed dra27@7bf0c44 which adds ocaml-config.2 (is the change similar to yours, I hope?!) and also dra27@529b7e7 which adds ocaml-base-compiler.4.12.0~alpha1

@Octachron
Copy link
Member

Yep, my changes in ocaml-config were equivalent. Are there any blockers left for a release in the beginning of next week?

@dra27
Copy link
Member

dra27 commented Nov 6, 2020

I don't believe so, no - just needs @AltGr to push the changes here, if you're happy for this PR to turn into the "OCaml 4.12.0 Alpha1" PR!

@Octachron
Copy link
Member

I think it is better to have an early release for this first alpha, and fix any remaining issues in the following alphas.

Not strictly needed, but that avoids a warning on opam 2.0; they will show up in 'opam switch list-available' though
@hannesm
Copy link
Member

hannesm commented Nov 9, 2020

FreeBSD was re-added to "CC=cc" {os = "openbsd" | os = "macos"}. If I remember correctly it was changed after a comment from @hannesm saying FreeBSD should not require that

Indeed, the OCaml configuration script nicely detects cc on FreeBSD (since 4.10.1), and the CC=cc shouldn't be needed (actually neither on macos nor OpenBSD, but someone with such a system should test this before removing it). See ocaml/ocaml#9437 and #16575 for details if interested.

Removing the conflict is OK because the 3 depend options are mutually exclusive
@dra27
Copy link
Member

dra27 commented Nov 9, 2020

Thanks, @kit-ty-kate! This PR is supposed to be an incremental step towards an ultimate neat solution so the intention with some of the corners has been to be paranoid about preserving existing use-cases. Thanks to @Octachron's agreement, the idea is to experiment with solely within the non-critical 4.12/4.13 development release packages, so from the OCaml-side we are willing to take a risk and if it turns out it's disastrous for some unforeseen reason then revert later as part of the alpha/beta/rc cycle.

  • The 4.13.0 package need updating too otherwise they'll break with the new changes in ocaml-config

There shouldn't be any breakage - what are you seeing? ocaml-config.2 merely adds the ability to detect ~ in the version string. The only difference here is that switches with ocaml-variants.4.13.0+trunk will ask to recompile if they're unlocked, but I view that as low-priority for the OCaml "dev-repo" package.

  • I don't think I like the move of the alpha packages from ocaml-variants to ocaml-base-compiler. I think it should stay stable releases only. Also it's not really consistent with having the +trunk package in ocaml-variants in my opinion.

I could do with a technical reason. The reason for correcting the semver (i.e. using ~alpha1 instead of +alpha1) is precisely to allow upgrades - that is, a switch on ocaml-base-compiler.4.12.0~alpha1 can naturally upgrade to alpha2 and on to release. As with previous versions, you can only end up with an alpha/beta/rc release if you explicitly request it (either by allowing ocaml-beta.enabled to be installed for opam 2.0 or by explicitly requesting 4.12.0~alpha1 as the version).

The inconsistency with +trunk is also an artefact of backwards compatibility. The +trunk packages themselves are an artefact of OPAM 1.x and the intention is to use --dev-repo (they're moving targets, so depending on ocaml-variants.4.13.0+trunk is largely meaningless) but that's an orthogonal change to this.

  • I think ocaml-option-* (singular) would fit better for the useflag packages than its plural counterpart

I like this idea!

  • For the ocaml-variants.4.12.0~alpha1+options package I think it would make more sense if the options were a disjunctions rather than depopts. It would be rather surprising to have a +options setup but no actual options.

@AltGr has also commented on this. The ultimate aim if we go with this experiment is that there would no longer be ocaml-base-compiler and ocaml-variants+options. The only reason for having the +options is to maximise the backwards compatibility which means keeping opam switch create 4.12.0~alpha1 consistent. We can potentially bike-shed the exact name for +options!

  • The vanilla packages should depend on the new ocaml-option-vanilla so that packages can request this and get the correct behaviour.

I don't completely follow - do you mean that ocaml-base-compiler et al should depend on it? In particular, my impression is that packages should be discouraged (or even prohibited) from depending on ocaml-option-vanilla - what package would you have in mind that has the "right" to say "I must be installed with a vanilla compiler" (for example, ruling out installation on 32bit or in a bytecode-only switch). Rather, such packages should explicitly conflict the option they cannot be built with (i.e. "I depend on ocaml-option-no-flat-float-array because I don't work without it" or "I conflict ocaml-option-nnp because I definitely use naked pointers")

  • The main issue brought up in [Discussion] New compiler flag scheme #16753 is still there: the option-* package style does not offer any guaranties that the set requested when creating the switch will stay the same. options could be change without user's consent by just one package requesting an option and next thing you know your pretty 4.12+flambda switch does not contain flambda anymore. To fix this I think we should make all the options packages incompatible with the other ones and add new one combining some of them.

There are ocaml-options-only packages to deal (I think) with this situation - more can be added if they're requested (indeed, if really wanted, they can all be generated with a script!).

  • Do we have a migration plan for opam 2.2 to switch this configuration to a USE-flag-like system? I think if the migration will includes a total rearrangement of the compiler packages it should be safe to merge this PR, however if it does not include it I think we should think twice about how it would work with the new features. I'm afraid this is going to bring more backward compatibility issues if this is not thought of before.

There's no need to block a test in pre-release versions of the package, though. I'm not particularly convinced that concern for a future opam feature needs to block changes to an opam 2.0 repository. Whatever feature is brought in, opam 2.0 is in LTS for longer than that, so either the new feature needs a compatibility story with the existing layout or it needs to be trivially possible to maintain both. The point with this layout - ignoring the hidden-version flag, is that it utilises existing opam 2.0 facilities.

@AltGr
Copy link
Member Author

AltGr commented Nov 12, 2020

Note: @camelus rejects the "new broken package: ocaml-option-spacetime.1", but that's intended, the option is provisioned for earlier compilers and this PR only adds 4.12, which doesn't support it.

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.

Another pass

packages/ocaml-variants/ocaml-variants.4.12.0+trunk/opam Outdated Show resolved Hide resolved
packages/ocaml-variants/ocaml-variants.4.12.0+trunk/opam Outdated Show resolved Hide resolved
"ocaml-base-compiler" {>= "4.12.0~" & < "4.12.1~"} |
"ocaml-variants" {>= "4.12.0~" & < "4.12.1~"} |
"ocaml-system" {>= "4.12.0" & < "4.12.1~"} |
"metaocaml" {>= "4.12.0~" & < "4.12.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 think the metaocaml line go in a separate PR as and when that's moved

Co-authored-by: David Allsopp <david.allsopp@metastack.com>
@dra27 dra27 marked this pull request as ready for review November 12, 2020 15:31
Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

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

Apart from this one small suggestion, after talking about this change more globally I think this experiment looks good to merge

depends: [
"ocaml" {= "4.12.0" & post}
"base-unix" {post}
"base-bigarray" {post}
"base-threads" {post}
"ocaml-beta"
"ocaml-beta" {opam-version < "2.1"}
]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
]
"ocaml-option-vanilla"
]

Copy link
Member

Choose a reason for hiding this comment

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

Note it's remained ocaml-options-vanilla (in plural) - so you depend on "vanilla options" (i.e. no options) rather than "the vanilla option" (which, alas, OCaml does not in fact have 🍦)

I think this is already achieved, though, because of the conflict-class, which prevents ocaml-base-compiler switching to ocaml-variants already, @AltGr?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, do we want to move the remaining ocaml-options-* to their singular counterparts before that? I think it's a bit confusing and potential source of errors to have both.

Apart from that, the request itself isn't a blocker, it's more for the sake of completeness. I don't care about it that much.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, indeed, in the case of ocaml-base-compiler, we have two options:

  • Have ocaml-base-compiler be just an alias for ocaml-variants.x+options & ocaml-options-vanilla
  • Have it stand-alone as before
  • (indeed 3: replace base-compiler by the version with options, not needing the variants.x+options at all. Not right away though)

The two should be equivalent in what you will get in the resulting switch ; my first take was option 1 here, to avoid duplication of the build instruction, but after some thought we decided to be more conservative and keep the stand-alone definition: it's less disruptive to current users of ocaml-base-compiler.

@dra27 yes, ocaml-variants and ocaml-base-compiler are mutually exclusive as before, and all options require variants so this is not needed; I guess @kit-ty-kate wanted the presence of options-vanilla to be a reliable marker that you are on the vanilla case; since the ocaml-options-vanilla package has already been made compatible with ocaml-base-compiler, there would be no harm in adding this dependency here to that purpose I guess.

@kit-ty-kate
Copy link
Member

Looks alright. Feel free to revert at any point if something is really broken.

Thanks!

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.

6 participants