-
Notifications
You must be signed in to change notification settings - Fork 96
require Arith and use RelationClasses (for stdlib#150 and stdlib#152) #821
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
require Arith and use RelationClasses (for stdlib#150 and stdlib#152) #821
Conversation
|
@andrew-appel if CI here confirms this is indeed backward compatible, could we please consider merging it so that the upstream PR on Stdlib can get merged |
|
The dev version of Rocq fails with: Should I go ahead and merge anyway? What would be a compatible proof for Injective_map_NoDup? |
|
Cc @andres-erbsen this doesn't seem perfectly backward compatible (would be better if we can avoid synchronous overlays) |
|
I can probably fix this next week. But what is a "synchronous overlay"? |
|
However, I find it curious that rocq-prover/stdlib#154 passed the stdlib CI job for VST. If we had noticed the breakage, we would have added a compatibility wrapper at the old path. Does VST CI build more of VST than stdlib ci? Should we build more? (@proux01 is this the right file to look at?) (If it's really a previous change to stdlib change that broke VST, i don't think a synchronous overlay would help here. This term refers to creating a pair of PRs that need to be merged "at the same time", e.g. to stdlib and VST. We have resorted to this in cases where compatibility with released versions is fine but it would be hard for e.g. VST to be compatible with stdlib master and stdlib master + the new stdlib PR at the same time.) |
|
It's likely that the rocq-prover's own CI does not run this "test2". Qroc's CI runs only a small subset of VST's CI. Can someone tell me what opam commands I would have to issue in a fresh switch to build the dev version of Croq and then build VST in it? In fact, I can't even build a fresh switch without knowing what version of OCaml it's supposed to use, so let me rephrase the question: What opam commands will build a switch with the dev version of Qorc such that VST will build in it? |
|
TL;DR: Looks like I first need to improve VST support in Stdlib CI, let me try to do it, hopefully by the end of the week.
I was under the assumption that the CI breakage here came from the current overlay not being backward compatible, making it a synchronous overlay: one that must be merged synchronously to the upstream PR since it is not backward compatible (that unfortunately happens a lot with Rocq plugins, but should be essentially avoidable for libraries) Anyway, our main issue indeed seems to be the CI of Stdlib not testing enough. At the very least we should ensure it tests as much as the CI of Rocq.
Rocq's CI essentially runs
I'd say should get you Rocq master + Stdlib master. You can even That being said, those dev packages are less maintained than the release repo so they can be a bit broken sometimes. |
|
@andres-erbsen XXX can you give me permission to push to this fork? Seems simpler than making a p.r. to your p.r. XXX Never mind about the permission, my local repo is in a state where I have to go forward with the p.r. to the p.r. Indeed, the fix you propose does work. I have also updated some submodules. After I merge this one into master, I plan a new p.r. with:
|
|
Closing without merging, because it is subsumed into #825. |
|
FYI I updated the Stdlib CI rocq-prover/stdlib#182 to align it with Rocq one, taht is basically doing |

Simple adaptation for rocq-prover/stdlib#152
I am not sure why I needed to Require Arith in the other file. Hopefully that change is acceptable regardless.
Thanks!
Edit: mystery solved:
Require Arithis for rocq-prover/stdlib#150