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

avoid notation clash with ssr to make examples-ssr build again #20

Merged
merged 3 commits into from Dec 8, 2020

Conversation

palmskog
Copy link
Member

@palmskog palmskog commented Dec 7, 2020

@co-dan here is a way to preserve the SSR examples as-is by fiddling with what is imported. Only the MathComp version of ssrfun exports the offending notations, so we just avoid it.

@RalfJung
Copy link
Collaborator

RalfJung commented Dec 7, 2020

@palmskog thanks! Could you rebase onto master and change this line to build the all target, so that CI ensures that this does indeed build?

FWIW, I don't really have a preference either way between #19 and this. Whatever is fine for Dan is also fine for me.

@co-dan
Copy link
Collaborator

co-dan commented Dec 7, 2020

Ah, so only the mathcomp's ssrfun was the offender? I just assumed that all the ssreflect modules from Coq are equivalent to the corresponding ssreflect modules from mathcomp, bu tturns out that's not the case...

I would prefer to have this over #19.

@palmskog
Copy link
Member Author

palmskog commented Dec 8, 2020

@RalfJung I did the rebase/changes you asked, and CI seems to pass fine. I don't think having a special CI opam package is a good long-term solution, but this is something to address in a future PR.

- version: '1.10.0-coq-8.11'
repo: 'mathcomp/mathcomp'
- version: '1.9.0-coq-8.10'
repo: 'mathcomp/mathcomp'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know the semantics of this field, but it looks a bit like this says we require mathcomp, when really we don't (just for one of the tests). Is that a potential problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

The way the all target works is that it will always require coq-mathcomp-ssreflect to be installed. To save time in CI, I select Docker images that already have the ssreflect package installed along with Coq. I doubt people will read the CI configuration to figure out dependencies (this is what the README and opam packages are for).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I saw the CI config changes in .github/workflows/coq-ci.yml; how is meta.yml related to that?

Copy link
Member Author

@palmskog palmskog Dec 8, 2020

Choose a reason for hiding this comment

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

I use our configuration templates to generate .github/workflows/coq-ci.yml based on the data in meta.yml. The boilerplate in the target yaml file is based on best practices for using the Coq Docker Action and maintained centrally. This is our response to the ever-changing configuration file requirements on GitHub and elsewhere.

@RalfJung
Copy link
Collaborator

RalfJung commented Dec 8, 2020

I don't think having a special CI opam package is a good long-term solution, but this is something to address in a future PR.

Other proposals to achieve this are welcome. :) I just had to find a way to make CI build something that should not be built on a regular opam install.

@palmskog
Copy link
Member Author

palmskog commented Dec 8, 2020

@RalfJung yes, I will do a separate proposal PR for improved opam/CI handling soon, but I believe this can be merged now.

@RalfJung
Copy link
Collaborator

RalfJung commented Dec 8, 2020

Sure, this looks good and Dan also agreed. Thank @palmskog!

@RalfJung RalfJung merged commit ad806d2 into master Dec 8, 2020
@RalfJung RalfJung deleted the fix-ssr branch December 8, 2020 10:21
@RalfJung RalfJung mentioned this pull request Dec 8, 2020
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.

None yet

3 participants