Conversation
Signed-off-by: Will Thomas <30wthomas@gmail.com>
Signed-off-by: Will Thomas <30wthomas@gmail.com>
rlepigre-skylabs-ai
left a comment
There was a problem hiding this comment.
I think this is a reasonable approach, although I would consider it inferior to falling back to rocq c --config if rocq --config fails (not sure how hard that would be to do). The current approach would make it harder for Rocq to remove the --config option from rocq c, which I think would make sense eventually (CLI flags are kind of a mess in Rocq). That being said, I'm fine if we want to simply merge this.
|
|
||
| test-all: $(BIN) | ||
| $(BIN) build @runtest @runtest-js @runtest-rocq @runtest-melange | ||
| DUNE_ROCQ_TEST=enable $(BIN) build @runtest @runtest-js @runtest-rocq @runtest-melange |
There was a problem hiding this comment.
Actually, I think its fine. Otherwise the runtest-rocq alias will not build. We don't use test-all in CI anyway.
|
@Durbatuluk1701 could you ask the Rocq maintainers on Zulip if they intend to change this flag in the future. That would probably give us the clearest path forward. |
Asked here |
|
The endorsed fix is:
Which I think should align with what this PR currently contributes |
|
Could you add a changelog entry? |
Signed-off-by: Will Thomas <30wthomas@gmail.com>
Reflecting on the issue in #13774, I think a solution that is both forwards and backwards compatible is to just always use
rocq c --config. This PR implements this change and allows compilation ofrocq.theorystanzas for Rocq 9.0+.Documentation in the Rocq CLI implies this will be recognized by
compile/cand the message for this commit seems to imply that--configwill need to access the environment regardless.Maybe there is some reason to utilize just
rocq --config(note: missing thec), but it is not immediately obvious to me.It working with PR dune vs. failing previous version