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

pkg: don't use "compiler" flag to identify compiler #10521

Merged

Conversation

gridbugs
Copy link
Collaborator

Opam's "compiler" flag is used too broadly to accurately identify packages containing ocaml compilers. For example this flag is set for compiler options packages such as ocaml-option-flambda which configure the ocaml-variants package, effectively preventing the use of compiler options packages, as ocaml-variants also has the "compiler" flag set, and dune only permits a single compiler package in a solution.

This change fixes this problem by using the presence of the "ocaml-core-compiler" conflict class to identify compiler packages.

@gridbugs gridbugs force-pushed the use-conflict-class-to-identify-compiler-packages branch from 8931609 to f3ba561 Compare May 13, 2024 13:35
Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

The implementation and reasoning sounds very sensible, probably our generated test compiler packages need to gain this conflict class (which makes sense, to be more accurate to what opam-repository actually does) and then its good to merge.

@gridbugs gridbugs force-pushed the use-conflict-class-to-identify-compiler-packages branch from f3ba561 to 5d18a2b Compare May 14, 2024 05:56
Opam's "compiler" flag is used too broadly to accurately identify
packages containing ocaml compilers. For example this flag is set for
compiler options packages such as ocaml-option-flambda which configure
the ocaml-variants package, effectively preventing the use of compiler
options packages, as ocaml-variants also has the "compiler" flag set,
and dune only permits a single compiler package in a solution.

This change fixes this problem by using the presence of the
"ocaml-core-compiler" conflict class to identify compiler
packages.

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
@gridbugs gridbugs force-pushed the use-conflict-class-to-identify-compiler-packages branch from 5d18a2b to 7171143 Compare May 14, 2024 05:59
@gridbugs
Copy link
Collaborator Author

Ok I've fixed the test

@rgrinberg
Copy link
Member

and dune only permits a single compiler package in a solution.

I suppose we could also lift this limitation. This fix should work as well though

@gridbugs gridbugs merged commit 28de4eb into ocaml:main May 14, 2024
27 of 28 checks passed
@gridbugs gridbugs deleted the use-conflict-class-to-identify-compiler-packages branch May 14, 2024 06:53
@Leonidas-from-XIV
Copy link
Collaborator

I suppose we could also lift this limitation.

I'm curious, how would that work? My fear is that if you mix and match compilers in your build, you get artifacts that aren't compatible with each other.

@rgrinberg
Copy link
Member

The conflict class already prevents invalid build plans, so that could never happen regardless.

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.

3 participants