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

Remove deprecated files in Arith and Numbers/Natural #18164

Merged
merged 6 commits into from Nov 28, 2023

Conversation

Villetaneuse
Copy link
Contributor

@Villetaneuse Villetaneuse commented Oct 15, 2023

  • Removed in Arith:

    • Div2.v
    • Even.v
    • Gt.v
    • Le.v
    • Lt.v
    • Max.v
    • Minus.v
    • Min.v
    • Mult.v
    • Plus.v
    • Arith_prebase.v
  • Removed Numbers/Natural/Peano/NPeano.v

  • Many "Require" statements involving these files removed in other parts of the library

  • Several deprecated lemmas removed in Arith/EqNat

  • Register commands moved from Arith_prebase to PeanoNat

  • arith hint database moved from Arith_prebase to Arith_base

  • Lemmas in Arith_base used only for the hint database have been blacklisted from Search

  • Some test files had to change:

    • bugs/bug_13307.v
    • bugs/bug_1779.v
    • bugs/bug_4187.v
    • bugs/bug_4456.v
    • modules/Nat.v
    • success/Funind.v
    • success/RecTutorial.v
    • success/Require.v (Not sure about this one)
    • success/extraction.v
    • success/import_lib.v
  • Added / updated test-suite.
  • Added changelog.
  • Added / updated documentation.

Overlays (to be merged before this PR)

@Villetaneuse Villetaneuse requested review from a team as code owners October 15, 2023 09:05
@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Oct 15, 2023
@Villetaneuse Villetaneuse removed request for a team October 15, 2023 09:06
@Villetaneuse Villetaneuse added kind: cleanup Code removal, deprecation, refactorings, etc. part: standard library The standard library stdlib. labels Oct 15, 2023
@Villetaneuse Villetaneuse requested a review from a team as a code owner October 15, 2023 09:50
@Villetaneuse Villetaneuse removed the request for review from a team October 15, 2023 10:30
@Villetaneuse
Copy link
Contributor Author

Since the CI is idle on Sunday, I'm running it (I hope it's ok, sorry if it's not) to evaluate how many overlays will be needed.

@Villetaneuse
Copy link
Contributor Author

@coqbot run full ci

@coqbot-app coqbot-app bot removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Oct 15, 2023
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Oct 15, 2023

The job library:ci-fiat_crypto_legacy has failed in allow failure mode
ping @JasonGross

@proux01
Copy link
Contributor

proux01 commented Oct 15, 2023

Wow, that sounds like a lot of work. Feel free to ping the various maintainers so that they update their code.

@proux01
Copy link
Contributor

proux01 commented Oct 15, 2023

I should personally handle paramcoq and I could have a look at Flocq.

@Villetaneuse
Copy link
Contributor Author

Wow, that sounds like a lot of work. Feel free to ping the various maintainers so that they update their code.

Don't you think I should wait for a review before starting these?

@Villetaneuse
Copy link
Contributor Author

I think I will have to shellscript my way out of this :D

@palmskog
Copy link
Contributor

Both ci-atbr and ci-verdi-raft should be compatible with this PR now.

@Villetaneuse
Copy link
Contributor Author

Villetaneuse commented Oct 15, 2023

Both ci-atbr and ci-verdi-raft should be compatible with this PR now.

  • ci-autosubst merged
  • ci-bbv submitted not yet merged
  • ci-math_classes merged

@proux01
Copy link
Contributor

proux01 commented Oct 16, 2023

Wow, that sounds like a lot of work. Feel free to ping the various maintainers so that they update their code.

Don't you think I should wait for a review before starting these?

Well, as far as I can see you are just removing things that were deprecated in 8.16 (three versions ago) so this should be consensual (I mean, if anyone opposes the deprecation, it should have already been discussed by now).

@Villetaneuse
Copy link
Contributor Author

Wow, that sounds like a lot of work. Feel free to ping the various maintainers so that they update their code.

Don't you think I should wait for a review before starting these?

Well, as far as I can see you are just removing things that were deprecated in 8.16 (three versions ago) so this should be consensual (I mean, if anyone opposes the deprecation, it should have already been discussed by now).

There could be a discussion about what is happening with Arith_prebase, the Search Blacklists and the modified test files.
But you're right, it does not harm to modify the external libs anyway.

proux01 added a commit to coq-community/paramcoq that referenced this pull request Oct 16, 2023
proux01 added a commit to coq-community/paramcoq that referenced this pull request Oct 16, 2023
@proux01
Copy link
Contributor

proux01 commented Oct 16, 2023

ci-paramcoq should now work and I have a merge request for Flocq that I'll submit when Inria gitlab is back tomorrow.

@Villetaneuse
Copy link
Contributor Author

ci-paramcoq should now work and I have a merge request for Flocq that I'll submit when Inria gitlab is back tomorrow.

Thank you very much!

Copy link
Contributor

coqbot-app bot commented Nov 27, 2023

🔴 CI failures at commit 9199776 without any failure in the test-suite

✔️ Corresponding jobs for the base commit cb76d6a succeeded

❔ Ask me to try to extract minimal test cases that can be added to the test-suite

🏃 @coqbot ci minimize will minimize the following targets: ci-cross_crypto, ci-fiat_crypto, ci-fiat_crypto_ocaml, ci-fiat_parsers
  • You can also pass me a specific list of targets to minimize as arguments.

So that they can be updated by their devs at their own pace
in the coming weeks.
@proux01 proux01 added the request: full CI Use this label when you want your next push to trigger a full CI. label Nov 27, 2023
@coqbot-app coqbot-app bot removed the request: full CI Use this label when you want your next push to trigger a full CI. label Nov 27, 2023
Copy link
Contributor

coqbot-app bot commented Nov 27, 2023

🔴 CI failures at commit 0521c8f without any failure in the test-suite

✔️ Corresponding jobs for the base commit cb76d6a succeeded

❔ Ask me to try to extract minimal test cases that can be added to the test-suite

🏃 @coqbot ci minimize will minimize the following targets: ci-cross_crypto, ci-fiat_crypto, ci-fiat_crypto_ocaml, ci-fiat_parsers
  • You can also pass me a specific list of targets to minimize as arguments.

@proux01 proux01 self-assigned this Nov 28, 2023
@proux01
Copy link
Contributor

proux01 commented Nov 28, 2023

As agreed during the last meeting, putting the last failing CI entries in allow_failure mode (so that their maintainers can take care of them at their own pace) and merging.

@coqbot merge now

@coqbot-app coqbot-app bot merged commit b06166b into coq:master Nov 28, 2023
11 checks passed
@JasonGross
Copy link
Member

JasonGross commented Nov 28, 2023

As agreed during the last meeting, putting the last failing CI entries in allow_failure mode (so that their maintainers can take care of them at their own pace) and merging.

I don't think this is reasonable, especially for Fiat Crypto.

  1. It seems to me that this violates the CI agreement:

Any time that a proposed change is breaking your project, Coq developers and contributors will send you patches to adapt it or will explain how to adapt it and work with you to ensure that you manage to do it.

  1. If I recall correctly, the last time a PR was merged that broke Fiat Crypto, in the intervening week that it took too fix Fiat Crypto, another PR broke Fiat Crypto in a different way without realizing it, because the CI wasn't testing Fiat Crypto
  2. I'm currently very busy and don't really have time to make intensive changes to these projects. If the changes are not intensive I would have appreciated you submitting PRs. If the changes are intensive I think we should reconsider the backwards compatibility policy with respect to the automation that should be provided for fixing changes.
  3. I don't understand what the rush is to merge this PR. As I understand it, there is nothing that depends on this PR, nothing blocked on this PR being merged, and merging this PR, while it does clean up the standard library somewhat, does not significantly improve Coq relative to the cost that it imposes such that it should be rushed into Coq at the cost of breaking a handful of CI developments.
  4. This breaks Fiat Crypto's deploy workflow, which was set up on the presumption that Coq wouldn't needlessly break our build.

I'm sorry that I haven't had the time to prepare patches for the projects I (co)maintain to adapt to this PR, but I'd rather see this reverted than leave the CI in a broken state, especially with projects that frequently catch incompatibilities with Coq.

@SkySkimmer
Copy link
Contributor

As agreed during the last meeting, putting the last failing CI entries in allow_failure mode (so that their maintainers can take care of them at their own pace) and merging.

Is that what we said? the notes say " We really aim to have it before the RC release." nothing about allow_failure

@proux01
Copy link
Contributor

proux01 commented Nov 29, 2023

Yes, I even remember checking during the wrapup at the end of the discussion that there was no disagreement on that point. The notes indeed lack precision.

@proux01
Copy link
Contributor

proux01 commented Nov 29, 2023

It seems to me that this violates the CI agreement:

Any time that a proposed change is breaking your project, Coq developers and contributors will send you patches to adapt it or will explain how to adapt it and work with you to ensure that you manage to do it.

I disagree, but we should probably edit that sentence as it seems to be often understood (not saying this is the case here) as implying that Coq devs are responsible for CI entries updates, which they are not.

It's also true that we had no effective way to reach out to CI entries maintainers, this should be improved by #18321

I don't understand what the rush is to merge this PR.

We want this in the upcoming RC, otherwise this will only land in the next one, six months later. Not merging PRs in reasonnable time can be discouraging for new contributors, particularly when they already put a lot of effort into them. We already exhausted a few contributors this way and have to be more proactive in merging PRs to avoid discouragement.

Villetaneuse added a commit to Villetaneuse/rupicola that referenced this pull request Nov 29, 2023
samuelgruetter added a commit to mit-plv/rupicola that referenced this pull request Nov 29, 2023
Villetaneuse added a commit to Villetaneuse/fiat that referenced this pull request Nov 30, 2023
Villetaneuse added a commit to Villetaneuse/fiat that referenced this pull request Dec 1, 2023
Villetaneuse added a commit to Villetaneuse/fiat-crypto that referenced this pull request Dec 1, 2023
JasonGross pushed a commit to Villetaneuse/fiat that referenced this pull request Dec 1, 2023
JasonGross pushed a commit to mit-plv/fiat that referenced this pull request Dec 1, 2023
JasonGross pushed a commit to mit-plv/fiat-crypto that referenced this pull request Dec 2, 2023
* adapt to coq/coq#18164

* Fix String.v

* fix WithBedrock/Proofs.v
Villetaneuse added a commit to Villetaneuse/coq that referenced this pull request Dec 2, 2023
JasonGross pushed a commit to JasonGross/fiat-crypto that referenced this pull request Dec 6, 2023
* adapt to coq/coq#18164

* Fix String.v

* fix WithBedrock/Proofs.v
@Villetaneuse Villetaneuse deleted the rm_arith_files branch January 9, 2024 14:37
nojb pushed a commit to LexiFi/menhir that referenced this pull request Apr 13, 2024
This merges MR #36.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: cleanup Code removal, deprecation, refactorings, etc. part: standard library The standard library stdlib.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants