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 warnings #720

Merged
merged 10 commits into from
Jun 28, 2022
Merged

Remove warnings #720

merged 10 commits into from
Jun 28, 2022

Conversation

yforster
Copy link
Member

No description provided.

@mattam82
Copy link
Member

There's still a bit of an issue with check_todos https://github.com/MetaCoq/metacoq/runs/7077275174?check_suite_focus=true#step:4:7

@yforster
Copy link
Member Author

Thanks! I think I fixed this now.

I'm a bit unsure yet how to include the checktodos. Ideally I'd like a 4th CI target which checks todos and fails if there are some, but does not affect the overall CI state of the PR. But I don't think GH actions allows such fine-grained notions

@yforster
Copy link
Member Author

I tried something, let's see

@mattam82
Copy link
Member

Also Set Warning does not seem to exist, only Set Warnings

@mattam82
Copy link
Member

Yes, seems like a thorny issue this allow-failure: actions/runner#2347

@yforster
Copy link
Member Author

Fixed!

I used allow-failure first, but then you have to click on the job to see that there are TODOs, which is a bit annoying. Now I'm trying to use if: always, but that's similarly annoying because CI becomes red when there are todos. So maybe go with the first variant where you have to click?

@mattam82
Copy link
Member

Having to click is fine I think

@yforster
Copy link
Member Author

One more try: I have a separate job which fails if there are todos, but the others are not aborted. This way the whole PR becomes red, but we can still inspect whether it compiles or not. I guess we never want to merge something with todo now, so maybe this is the way to go? Happy to revert to the click solution though

@yforster
Copy link
Member Author

Do we like this? #722

@mattam82
Copy link
Member

I like it as in actions/toolkit#722

@yforster
Copy link
Member Author

Then you can merge this PR here

@mattam82 mattam82 merged commit f6d8e17 into MetaCoq:coq-8.16 Jun 28, 2022
mattam82 pushed a commit that referenced this pull request Jun 29, 2022
* remove warnings

* remove warnings and remove axion on eta expansion

* fix build.yml

* fix build.yml

* fix compilation

* use bash for checktodos.sh

* always run CI even if there are todos

* improve output

* Set Warnings

* separate todo job
mattam82 pushed a commit that referenced this pull request Jul 1, 2022
* remove warnings

* remove warnings and remove axion on eta expansion

* fix build.yml

* fix build.yml

* fix compilation

* use bash for checktodos.sh

* always run CI even if there are todos

* improve output

* Set Warnings

* separate todo job
mattam82 pushed a commit that referenced this pull request Jul 1, 2022
* remove warnings

* remove warnings and remove axion on eta expansion

* fix build.yml

* fix build.yml

* fix compilation

* use bash for checktodos.sh

* always run CI even if there are todos

* improve output

* Set Warnings

* separate todo job
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