Skip to content

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

Closed
digama0 wants to merge 2 commits intomasterfrom
shake_ci
Closed

[Merged by Bors] - feat: add lake exe shake to CI#9751
digama0 wants to merge 2 commits intomasterfrom
shake_ci

Conversation

@digama0
Copy link
Copy Markdown
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 CI Modifies the continuous integration setup or other automation labels Jan 15, 2024
@ghost ghost added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Jan 23, 2024
@ghost ghost removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) 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
@kim-em
Copy link
Copy Markdown
Contributor

kim-em 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

urkud commented Jan 29, 2024

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

@digama0
Copy link
Copy Markdown
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.

@kim-em
Copy link
Copy Markdown
Contributor

kim-em commented Jan 30, 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.

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
Copy Markdown
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.)

@ghost ghost added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Feb 2, 2024
@ghost ghost removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Feb 2, 2024
@digama0
Copy link
Copy Markdown
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

@ghost
Copy link
Copy Markdown

ghost commented Feb 2, 2024

@jcommelin
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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 added the delegated This pull request has been delegated to the PR author (or occasionally another non-maintainer). label Feb 2, 2024
@digama0
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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 setup or other automation delegated This pull request has been delegated to the PR author (or occasionally another non-maintainer). ready-to-merge This PR has been sent to bors.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants