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

feat: Take advantage of the coq-native package (CEP 048, item 3) #1835

Merged
merged 1 commit into from
Oct 13, 2021

Conversation

erikmd
Copy link
Member

@erikmd erikmd commented Sep 28, 2021

Cc @proux01 @CohenCyril @strub

* href: https://github.com/coq/ceps/blob/master/text/048-packaging-coq-native.md#regarding-item-3

* Only update coq-mathcomp-multinomials.dev's deps and mathcomp.1.12.0
  (some similar PRs could be opened for other deps if need be).
erikmd added a commit to erikmd/multinomials that referenced this pull request Sep 28, 2021
* can be reverted anytime after the merge of
  coq/opam#1835
  and the rebuild of `mathcomp/mathcomp:1.12.0-coq-8.12`
@erikmd
Copy link
Member Author

erikmd commented Sep 28, 2021

BTW @gares I've just seen the related PR #1504 had not been integrated yet; do you think it can be rebased (to re-test it with GItHub Actions), and maybe merged before #1835?

strub pushed a commit to math-comp/multinomials that referenced this pull request Sep 29, 2021
@erikmd
Copy link
Member Author

erikmd commented Oct 12, 2021

FTR, even if the two main jobs timed out: 4.05.0, 4.11.2, it can be noted that the compilation did not raise any failure, see e.g.:

Failures so far:
Successes so far: coq-mathcomp-algebra.1.12.0 coq-mathcomp-bigenough.1.0.0 coq-mathcomp-character.1.12.0 coq-mathcomp-field-extra.1.6.1 coq-mathcomp-field.1.12.0 coq-mathcomp-fingroup.1.12.0 coq-mathcomp-finmap.1.5.0 coq-mathcomp-finmap.1.5.1
Packages that failed to install:
Packages that succeeded to install: coq-mathcomp-algebra.1.12.0 coq-mathcomp-bigenough.1.0.0 coq-mathcomp-character.1.12.0 coq-mathcomp-field.1.12.0 coq-mathcomp-fingroup.1.12.0 coq-mathcomp-finmap.1.5.0 coq-mathcomp-finmap.1.5.1 coq-mathcomp-solvable.1.12.0 coq-mathcomp-ssreflect.1.12.0

So I guess that the PR could be merged as is?

Anyway, let me know if you need more tests (maybe on a longer pipeline I'd run on a fork…), or if you'd like that #1504 is finalized beforehand.

Actually AFAICT, the tests that were run in this pipeline already confirm that adding the predicate "COQEXTRAFLAGS+=-native-compiler yes" {coq-native:installed & coq:version < "8.13~" } did not impact the standard installation (without the coq-native package) in any unexpected way (as the {coq-native:installed} predicate is just false in this case).

@palmskog
Copy link
Contributor

palmskog commented Oct 12, 2021

I don't feel I have enough understanding of coq-native to review this PR. Perhaps @gares or @silene could weigh in on whether we can merge this right now?

@silene
Copy link
Contributor

silene commented Oct 12, 2021

This pull request is fine with me. Unless @gares objects, I will merge it.

@gares
Copy link
Member

gares commented Oct 12, 2021

I'm fine with it. Note that you can re-run CI skipping previously checked packages, see https://github.com/coq/opam-coq-archive#continuous-integration

@gares
Copy link
Member

gares commented Oct 12, 2021

I did re-run it with a larger timeout, see https://gitlab.com/coq/opam-coq-archive/-/pipelines/386997682 . It should suffice.

@gares
Copy link
Member

gares commented Oct 12, 2021

Dear @coq/ci-maintainers it seems pyrolise is no more available to opam-coq-archive. It is listed as a new runner, which did not connect yet, and is locked which I don't know what that means. Can you shed some light?

@SkySkimmer
Copy link
Contributor

cc @maximedenes @ppedrot

@Zimmi48
Copy link
Member

Zimmi48 commented Oct 12, 2021

I talked about this during a recent Coq Call: https://github.com/coq/coq/wiki/Coq-Call-2021-09-08
I said I was not able to set it up again after it was upgraded to Ubuntu 20.04.
@silene volunteered to try it out but he didn't have access to the server yet. I wrote to Jean-Claude Soret (with whom I discussed previously about the upgrade) to ask him to give access to Guillaume but I didn't get any answer.
I can try asking again.

@erikmd
Copy link
Member Author

erikmd commented Oct 12, 2021

Just FTR (to illustrate the impact of this PR, using the opam pinned from the dev version of coq-mathcomp-multinomials),
this PR now makes it possible to install the following packages at once, with Coq 8.12:

coq                       8.12.0        pinned to version 8.12.0
coq-bignums               8.12.0        Bignums, the Coq library of arbitrary large numbers
coq-mathcomp-algebra      1.12.0        Mathematical Components Library on Algebra
coq-mathcomp-bigenough    1.0.0         A small library to do epsilon - N reasonning
coq-mathcomp-fingroup     1.12.0        Mathematical Components Library on finite groups
coq-mathcomp-finmap       1.5.1         Finite sets, finite maps, finitely supported functions
coq-mathcomp-multinomials 1.5.4         pinned to version 1.5.4 at git+https://github.com/math-comp/multi
coq-mathcomp-ssreflect    1.12.0        Small Scale Reflection
coq-native                1             Package flag enabling coq's native-compiler flag
cppo                      1.6.6         Code preprocessor like cpp for OCaml
csexp                     1.3.2         Parsing and printing of S-expressions in Canonical form
dot-merlin-reader         3.4.1         Reads config files for merlin
dune                      2.8.0         Fast, portable, and opinionated build system
easy-format               1.3.2         High-level and functional interface to the Format module of the O
merlin                    3.4.1         Editor helper, provides completion, typing and source browsing in
…

and to check this I've just did a local test:

eval $(opam env)
opam repo add coq-local-released file://$HOME/forge/git/opam-coq-archive/released
opam pin add -n -y -k git coq-mathcomp-multinomials "https://github.com/math-comp/multinomials.git#master"
opam install -j 2 coq-native coq-mathcomp-multinomials
opam list

(Cc @proux01 FYI)

@palmskog
Copy link
Contributor

It seems we did get some kind of consensus to merge in spite of CI issues, so therefore I will merge.

@palmskog palmskog merged commit 400b910 into coq:master Oct 13, 2021
@erikmd erikmd deleted the use-coq-native branch October 13, 2021 18:42
@CohenCyril
Copy link
Contributor

CohenCyril commented Oct 14, 2021

It looks that this breaks several CIs relying on big_enough, e.g.

CC @thery @erikmd
CC @gares (thanks for pointing me towards this PR)

@silene
Copy link
Contributor

silene commented Oct 14, 2021

Given the log, it seems like you are not compiling SSReflect but using a precompiled one (from before this pull request). It will keep failing until this precompiled SSReflect is recompiled.

@CohenCyril
Copy link
Contributor

CohenCyril commented Oct 14, 2021

Given the log, it seems like you are not compiling SSReflect but using a precompiled one (from before this pull request). It will keep failing until this precompiled SSReflect is recompiled.

It's the one from docker... so you're saying I just need to rebuild the docker image?

@silene
Copy link
Contributor

silene commented Oct 14, 2021

Yes, whoever is the maintainer of the Docker images you are using in your continuous integration should rebuild them (after disabling the cache).

@erikmd
Copy link
Member Author

erikmd commented Oct 14, 2021

Indeed, that's exactly what @silene said: it's not a bug but an expected failure to be solved by recompiling these mathcomp stable images since this PR was merged yesterday (sorry for not doing this earlier today).
I've just started this in this pipeline: https://gitlab.com/math-comp/docker-mathcomp/-/pipelines/388444664

@erikmd
Copy link
Member Author

erikmd commented Oct 14, 2021

@erikmd
Copy link
Member Author

erikmd commented Oct 14, 2021

@CohenCyril
Copy link
Contributor

Thanks @erikmd !!

erikmd added a commit to erikmd/multinomials that referenced this pull request Oct 14, 2021
strub pushed a commit to math-comp/multinomials that referenced this pull request Oct 15, 2021
* Follow-up of 52988b6

* Now possible after the merge of coq/opam#1835
erikmd added a commit to erikmd/opam-coq-archive that referenced this pull request Oct 18, 2021
erikmd added a commit to erikmd/math-comp that referenced this pull request Oct 18, 2021
thery pushed a commit to thery/multinomials that referenced this pull request Jan 3, 2023
thery pushed a commit to thery/multinomials that referenced this pull request Jan 3, 2023
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.

7 participants