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

Rm warnings 8.11 #43

Merged
merged 5 commits into from
Mar 29, 2022
Merged

Rm warnings 8.11 #43

merged 5 commits into from
Mar 29, 2022

Conversation

ybertot
Copy link
Collaborator

@ybertot ybertot commented Mar 25, 2022

This takes a subset of the changes in #41 and preserves compatibility with coq-8.11 and math-comp 1.11

@ybertot
Copy link
Collaborator Author

ybertot commented Mar 28, 2022

@palmskog can you help me? I don't understand why no CI check have been performed for this PR

@palmskog
Copy link
Member

@ybertot there was a GitHub outage this Friday that affected CI. The CI will run again if you do a new push (or I can do it if you want).

@ybertot
Copy link
Collaborator Author

ybertot commented Mar 28, 2022

Only a push can trigger it?

@palmskog
Copy link
Member

To my knowledge, there is no other way. But recall that you can do the following locally to do a "no-operation" push in the branch:

git commit --amend
git push -f

The first command changes the SHA of the last commit without changing its content, and then you push the new state to the upstream branch.

@ybertot
Copy link
Collaborator Author

ybertot commented Mar 28, 2022

thks, I just discovered I had modification I had forgotten to push anyway.

@palmskog
Copy link
Member

@ybertot this PR looks good now in CI. However, I think we should also stop to ignore the warnings that you fixed here, for example, the following should be removed in _CoqProject and dune files:

-arg -w -arg -implicit-core-hint-db
-arg -w -arg -duplicate-clear

@ybertot
Copy link
Collaborator Author

ybertot commented Mar 28, 2022

I don't understand. these lines do not appear in _CoqProject, and there is no dune file.

@ybertot
Copy link
Collaborator Author

ybertot commented Mar 28, 2022

@palmskog

@palmskog
Copy link
Member

palmskog commented Mar 28, 2022

Ah, my mistake, I saw some changes I had done locally. But then we may actually want to add the following into _CoqProject:

-arg -w -arg -ambiguous-paths
-arg -w -arg -deprecated-hint-without-locality
-arg -w -arg -deprecated-instance-without-locality
-arg -w -arg -deprecated-syntactic-definition

... to make the project warning-free during local compilation, and then remove those as more warnings are fixed.

@ybertot
Copy link
Collaborator Author

ybertot commented Mar 28, 2022

I had the same interrogations myself. What should be the policy: strive for a warning free compilation at the risk of forgetting the work that needs to be done as deprecation looms, or leave the deprecation warnings (and only these) as a reminder?

I would prefer to silence only the ambiguous-path warning. Would you agree?

@palmskog
Copy link
Member

palmskog commented Mar 28, 2022

My personal preference is for a warning-free build, with carefully curated GitHub issues and/or draft pull requests to document why some warnings are not addressed (likely due to Coq version compatibility). The main reason is that contributors to a project will not be distracted by irrelevant warnings locally or in the CI.

Still, silencing just the ambiguous-path warning is good as well.

@ybertot
Copy link
Collaborator Author

ybertot commented Mar 28, 2022

This made me discover a couple of bugs in the current setup:

  1. Makefile.coq.local should never have been checked in, this file is the one that should be used by developers to have their personal choice of warnings removed. The warnings in Makefile.coq.local should be included in the _CoqProject file.
  2. Makefile.coq.local refers to a variable OTHERFAGS when it should have been OTHERFLAGS. Because of this, any line of the form "-arg -w -arg -ambiguous-paths" for example, is actually ignored at compilation time.

@ybertot
Copy link
Collaborator Author

ybertot commented Mar 28, 2022

I don't know why Makefile.coq.local is not on the master branch, but it is on both my warning removal branches.

@ybertot
Copy link
Collaborator Author

ybertot commented Mar 28, 2022

Ok, I found out that @palmskog cleaned up the setup in PR #38, where the flawed Makefile.coq.local was actually removed from the main branch, but this branch has not been rebased yet, so it is still present.

_CoqProject Outdated Show resolved Hide resolved
Copy link
Member

@palmskog palmskog left a comment

Choose a reason for hiding this comment

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

This now looks ready to merge to me, it's a strict improvement given our current bounds on Coq and MathComp.

@ybertot ybertot merged commit 4486796 into coq-community:master Mar 29, 2022
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.

2 participants