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

[Merged by Bors] - feat: add lake exe shake to CI #9751

Closed
wants to merge 2 commits into from
Closed

Conversation

digama0
Copy link
Member

@digama0 digama0 commented Jan 15, 2024

This checks files for unused imports. The output here is piped through gh-problem-matcher-wrap so that it will show up as annotations.


Open in Gitpod

@digama0 digama0 force-pushed the shake_ci branch 2 times, most recently from 920af90 to 3915f97 Compare January 15, 2024 03:31
@digama0 digama0 added awaiting-review The author would like community review of the PR CI Modifies the continuous integration / deployment setup labels Jan 15, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Jan 23, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Jan 23, 2024
@digama0 digama0 force-pushed the shake_ci branch 3 times, most recently from a529ea8 to 699b03d Compare January 23, 2024 22:39
@semorrison
Copy link
Contributor

semorrison commented Jan 24, 2024

I think we need more straightforwardly human readable output. e.g.

./././Mathlib/GroupTheory/Subgroup/ZPowers.lean:
  remove #[Mathlib.Algebra.GroupPower.Lemmas]
  fix Mathlib.Algebra.GroupPower.Lemmas: #[Mathlib.Analysis.SpecificLimits.Basic, Mathlib.Data.Complex.Exponential]

should become:

In the file Mathlib/GroupTheory/Subgroup/ZPowers.lean, we can remove:
```
import Mathlib.Algebra.GroupPower.Lemmas
```
after which we'll add that import back into the following files:
* Mathlib/Analysis/SpecificLimits/Basic.lean
* Mathlib/Data/Complex/Exponential.lean

@digama0
Copy link
Member Author

digama0 commented Jan 24, 2024

I was hoping people would be using the github annotations, which are a bit more user friendly. The CLI output is more optimized for batch usage, where that kind of verbosity is a bit overwhelming if there are 30-100 of them. You can also just ignore the CLI output entirely and use lake exe shake --fix > /dev/null || git diff HEAD instead.

@urkud
Copy link
Member

urkud commented Jan 25, 2024

You can also just ignore the CLI output entirely and use lake exe shake --fix > /dev/null || git diff HEAD instead.

Can we turn this into clickable suggestions like we have for some style linters? Or it will fail because github won't accept suggestions to the lines that aren't touched by the PR?

@Vierkantor
Copy link
Contributor

You can also just ignore the CLI output entirely and use lake exe shake --fix > /dev/null || git diff HEAD instead.

That's definitely not something a user newly encountering shake is going to know, or teachable to the average mathematician. Before we encourage everyone to use this tool, we should make sure it doesn't raise further questions. Especially the output of --fix appearing to be in the imperative mood and the meaning of "fix" in the output looks confusing, and should be easy to fix. I've opened #9996 with my proposal.

@urkud
Copy link
Member

urkud commented Jan 29, 2024

What does --gh-style do and where can I see the output it generates?

@digama0
Copy link
Member Author

digama0 commented Jan 29, 2024

--gh-style produces additional output from shake (which you can see in the build logs), these are lines that look like file:line:col: warning: ... and shake does not normally produce them. The purpose is that this particular error format is understood by gh-problem-matcher-wrap, and it uses them to produce annotations that appear on the PR itself next to other lint warnings. Just look for the comments about unused import (use `lake exe shake --fix` to fix this... in https://github.com/leanprover-community/mathlib4/pull/9751/files, and the annotations also appear on the build summary page and are highlighted in yellow in the build log itself.

@semorrison
Copy link
Contributor

I was hoping people would be using the github annotations, which are a bit more user friendly. The CLI output is more optimized for batch usage, where that kind of verbosity is a bit overwhelming if there are 30-100 of them. You can also just ignore the CLI output entirely and use lake exe shake --fix > /dev/null || git diff HEAD instead.

I think we either need to have human readable output, or entirely suppress the non human readable output from the CI logs when you click on a red X! How is a new contributor meant to know they are meant to ignore this output?

@digama0
Copy link
Member Author

digama0 commented Jan 30, 2024

Does #9996 address your concerns? (The output is of course human readable and useful, so I definitely would not want to suppress it, it's just not exactly intended to be a tutorial about itself. Most other CI logs are at a similar level of verbosity.)

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Feb 2, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Feb 2, 2024
@digama0
Copy link
Member Author

digama0 commented Feb 2, 2024

This disables batching for this PR, so that it doesn't race with other PRs:

bors single on

@jcommelin
Copy link
Member

Thanks 🎉

If CI passes, please remove the label awaiting-CI and merge this yourself, by adding a comment bors r+.

bors d+

@mathlib-bors
Copy link

mathlib-bors bot commented Feb 2, 2024

✌️ digama0 can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@github-actions github-actions bot removed the awaiting-review The author would like community review of the PR label Feb 2, 2024
@digama0
Copy link
Member Author

digama0 commented Feb 2, 2024

bors r+

@github-actions github-actions bot added the ready-to-merge This PR has been sent to bors. label Feb 2, 2024
mathlib-bors bot pushed a commit that referenced this pull request Feb 2, 2024
- [X] depends on: #9830
- [X] depends on: #9772
- [X] depends on: #9996

This checks files for unused imports. The output here is piped through `gh-problem-matcher-wrap` so that it will show up as annotations.



Co-authored-by: Mario Carneiro <di.gama@gmail.com>
@mathlib-bors
Copy link

mathlib-bors bot commented Feb 2, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title feat: add lake exe shake to CI [Merged by Bors] - feat: add lake exe shake to CI Feb 2, 2024
@mathlib-bors mathlib-bors bot closed this Feb 2, 2024
@mathlib-bors mathlib-bors bot deleted the shake_ci branch February 2, 2024 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Modifies the continuous integration / deployment setup delegated ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants