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

fixup #775 #781

Merged
merged 1 commit into from
Jun 27, 2024
Merged

fixup #775 #781

merged 1 commit into from
Jun 27, 2024

Conversation

gares
Copy link
Member

@gares gares commented Jun 7, 2024

that was clearly incomplete

@gares
Copy link
Member Author

gares commented Jun 7, 2024

@rtetley we need a CI job with warn-as-errors, in elpi I do have:

https://github.com/LPCIC/elpi/blob/702576e6521a6a5b5ed2dfa5886d0de704552af4/dune#L12-L13

and then a job

https://github.com/LPCIC/elpi/blob/702576e6521a6a5b5ed2dfa5886d0de704552af4/.github/workflows/main.yml#L24-L27

and a run line that calls dune with --profile ${{ matrix.profile }}. Ideally it should use a very recent ocaml version, since new warnings are constantly added.

@gares
Copy link
Member Author

gares commented Jun 7, 2024

Such a ci job would have detected the author of the PR did introduce warnings (that happened to be errors)

@gares gares changed the title fixup https://github.com/coq-community/vscoq/pull/775 fixup #775 Jun 7, 2024
@rtetley
Copy link
Collaborator

rtetley commented Jun 7, 2024

I agree, I'll take care of it. They merged the PR without the CI working anyways so... Should we go back to having a coq-master branch ?

@rtetley
Copy link
Collaborator

rtetley commented Jun 7, 2024

Something like #782 ?

@gares
Copy link
Member Author

gares commented Jun 7, 2024

I agree, I'll take care of it. They merged the PR without the CI working anyways so... Should we go back to having a coq-master branch ?

I think we should make sure that the coq ci job does not break the other supported versions. My guess is that if we put the sources in a directory together with the stable version of Coq and we run dune build check we can do that quite fast (like 1 minute). Then we should detect this error and print "Use optcomp: put the change in a [%%if] block".

@SkySkimmer WDYT?

@SkySkimmer
Copy link
Contributor

IDK

@gares
Copy link
Member Author

gares commented Jun 9, 2024

with this it works: ocaml/dune#10631 in less than 1 minute
until it is merged, we cannot do much better than the status quo

@gares gares merged commit ae5b388 into main Jun 27, 2024
16 checks passed
@gares gares deleted the fix-e branch June 27, 2024 08:33
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