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 the switch 5.0.0+tsan #22651

Merged
merged 1 commit into from
Dec 14, 2022
Merged

Conversation

OlivierNicole
Copy link
Contributor

This is a new #22627. The difference is that we have fixed a problem in the pointed repo (namely macOS build) and a problem in the present opam file. In addition, this opam file now points to a 5.0.0+tsan branch, that can be updated with bugfixes, rather than to a fixed tag.

This will notably allow this switch to be updated when the final release of OCaml 5.0.0 is out, without requiring a new PR.

@kit-ty-kate kit-ty-kate merged commit 6cd0a45 into ocaml:master Dec 14, 2022
@kit-ty-kate
Copy link
Member

Thanks. Sorry for the wait

@OlivierNicole
Copy link
Contributor Author

Thank you!

@OlivierNicole OlivierNicole deleted the tsan-switch-2 branch December 14, 2022 20:03
@gasche
Copy link
Member

gasche commented Dec 17, 2022

I'm excited by ThreadSanitized (great work!), but I'm uneasy with having a switch called 5.0.0+foo that does not point to a (specific configuration) of the upstream 5.0 compiler. In the past, switches to experimental forks of the compiler distribution were in a separate repository (the multicore switches) or separate package names (ocaml-freestanding).. In fact the options system enforces this property by construction -- which is why the present switch couldn't use it.

Could we discuss this with upstream first?

(cc @OlivierNicole @kit-ty-kate @dra27 @Octachron)

@avsm
Copy link
Member

avsm commented Dec 17, 2022

There is precedent for these 'floating compiler patches' in opam-repository (4.00.1+open-types isn't entirely from a released 4.00, and there are some others mainly from older releases of the compiler), but I do agree with @gasche that it's a good time to tighten up our rules and conventions in this space with how we use the opam repository for this.

One approach might would be to request that compiler patches in the opam-repository are extra-files patches that apply over a released version. However, this would not work with compiler changes that require bootstrapping due to the binary files. A property it would be really nice to have in the mainstream opam-repository is for a released compiler, all of our packages do have checksums and not floating git branches.

@OlivierNicole
Copy link
Contributor Author

Sorry about this—happy to make the changes necessary to better respect naming and tagging conventions.

@Octachron
Copy link
Member

@gasche , the MetaOCaml patched compilers were also distributed as 4.00.1+BER.

But I agree that the use of ocaml-variants for fine-tuning the configuration of the official compiler muddled up the meaning of the ocaml-variants packages.

Maybe it could work to have non-official-ocaml-variants packages to draw a more strict separation? At the same time, one could also argue that this is the ocaml-variants*+options packages that are slightly out of place and that those packages could be merged with the main ocaml package as the only official OCaml packages.

@dra27
Copy link
Member

dra27 commented Dec 17, 2022

There's work in motion for the variant configurations of the upstream compiler exiting the ocaml-variants package, possibly allowing the "ocaml-variants" package really to mean "variants of OCaml" as opposed to "non-default configurations of OCaml".

There's a structural point that's not been explicitly mentioned so far. ocaml-freestanding is not a switch compiler in the same way. OCaml packages depend on the ocaml package which is provided by either ocaml-system, ocaml-variants or ocaml-base-compiler. So either the package has to be put in ocaml-variants, or we have to update the ocaml package to recognise an additional possible package here:

"ocaml-base-compiler" {>= "5.0.0~" & < "5.0.1~" } |
"ocaml-variants" {>= "5.0.0~" & < "5.0.1~"} |
"ocaml-system" {>= "5.0.0" & < "5.0.1~"}

which is an invasive change for opam 2.0 and 2.1 users (as switches recompile).

@gasche
Copy link
Member

gasche commented Dec 18, 2022

I think we could have an ocaml-experimental-variants that would be listed as one way to satisfy the virtual ocaml package indeed. If we only add it for 5.0 for now, the impact on users will be fairly limited.

An indirect benefit from not presenting tsan as a compiler variant is that it could presuambly use its own versioning scheme, instead of showing only the forked compiler's version number. This would let you release a new version of the tsan for for the same compiler version.
(Speaking of: another notable thing about +tsan is the practice of pointing to a tag rather than a branch, which is convenient but also limits reproducibility. We use it for the weird +trunk switches, but ideally there would be some way to tell whether packages are standard/stable or they are moving targets. Maybe moving trunk compilers to ocaml-experimental-variants could also clarify this.)

@OlivierNicole
Copy link
Contributor Author

An indirect benefit from not presenting tsan as a compiler variant is that it could presuambly use its own versioning scheme, instead of showing only the forked compiler's version number.

This has indeed been a source of worry for me: how to release a new tsan that is not associated to a compiler release.

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