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

Resync coq/coqide dev packages with (safer, slower) release settings #1354

Closed
wants to merge 2 commits into from

Conversation

Blaisorblade
Copy link
Contributor

These opam packages are currently used as templates for releases.
See discussion at ocaml/opam-repository#16908,
ocaml/opam-repository#16876 (comment)
and
coq/coq#11178 (comment).

Only done for 8.12 and dev, which will be used in the future.

The resulting packages are safer and slower (for flambda users); fixing that
will require a more proper fix (such as ocaml/opam-repository#16887).

These opam packages are currently used as templates for releases.
See discussion at ocaml/opam-repository#16908,
ocaml/opam-repository#16876 (comment)
and
coq/coq#11178 (comment).

Only done for 8.12 and dev, which will be used in the future.

The resulting packages are safer and slower (for flambda users); fixing that
will require a more proper fix (such as ocaml/opam-repository#16887).
Warning: haven't tested the comment syntax.
@@ -33,7 +33,8 @@ build: [
"-libdir" "%{lib}%/coq"
"-datadir" "%{share}%/coq"
"-coqide" "no"
"-flambda-opts" "-O3 -unbox-closures"
# These settings are used as templates for release packages
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the point of this comment here. If native-compiler is broken on macOS, I'd expect a comment pointing to why it is broken. Whether or not they are sued for release packages is irrelevant IMO; even dev packages should work on macos after all.

Copy link
Contributor

@RalfJung RalfJung Jul 29, 2020

Choose a reason for hiding this comment

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

Likewise, "unbox-closures" is not set because it is broken, so the justification should be a link to the issue for that, not "this is a template for releases".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. My comments was meant for the current users of these packages, whose revealed preferences seem different.

This PR could also wait for ocaml/opam-repository#16887 to be finalized and sync with that, since the solution there should be much better, and would be needed even if we fixed all current bugs.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems to still enable flambda for ocaml >= 4.07 -- so only older ocaml is broken?

Indeed if that PR is likely to be accepted, it makes sense to wait for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That enables flambda if you picked an flambda compiler (else those options are ignored).

so only older ocaml is broken?

flambda+native is also "YMMV", depending on multiple factors, but native must also be selected explicitly.

The story for native is even longer and I'll spare you, but the plan (in coq/coq#12757) includes producing sufficient docs; eventually, enabling native should precompile native versions of all libraries where it makes sense.

@Blaisorblade Blaisorblade marked this pull request as draft July 29, 2020 13:56
@Blaisorblade
Copy link
Contributor Author

Also no idea why CI failed, hope that's a spurious failure; investigation can wait.

@gares
Copy link
Member

gares commented Jul 29, 2020

for some reasons coqide.dev failed to install, but I don't get why from the log

Packages that failed to install: coqide.dev
Packages that succeeded to install: coq.8.12.dev coq.dev coqide.8.12.dev

It seems to build correctly

@Blaisorblade
Copy link
Contributor Author

Blaisorblade commented Jul 29, 2020

Disk out of space? EDIT: or permission error (but that'd be worse).

@palmskog
Copy link
Collaborator

@Blaisorblade are we going to see any progress on this PR in the near future? The same error occurs on several runners, so it's not clear to me this is a spurious error.

@Blaisorblade
Copy link
Contributor Author

Blaisorblade commented Aug 17, 2020

I don’t see how the error could be due to this PR. EDIT: it could be a valid underlying error.

This was marked as draft while waiting for ocaml/opam-repository#16887, but that’s also stalled.

@Blaisorblade
Copy link
Contributor Author

But I don’t have to follow up on this in the near future, nor do I see anything I can follow up on. So closing; feel free to resurrect.

@RalfJung
Copy link
Contributor

@palmskog This is sure going to lead to more confusion and issues down the road when these packages are not in sync with the ocaml repo.

@RalfJung
Copy link
Contributor

Opened #1389.
Those opam files are the same as for the current stable release -- if CI still fails, it's more likely a CI issue than a package issue.

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.

4 participants