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

Add PHONY rules for lint and lint-json #10584

Merged
merged 1 commit into from
Nov 24, 2024

Conversation

philderbeast
Copy link
Collaborator

Fixes #10583, adding PHONY rules for lint and lint-json.

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

I'm generally not sure how much profit we get from such simplistic one-line, three-four-words make rules. This saves very little but makes Makefile bigger and harder to maintain. But I'm not opposed to it. Myself, I don't really use the Makefile.

One more thing I always say about new targets in the Makefile: consider updating the CI to use these targets if you don't want them to bitrot right away.

@philderbeast
Copy link
Collaborator Author

This saves very little

It can save time:

$ time hlint .
No hints

________________________________________________________
Executed in   92.47 secs    fish           external
   usr time   92.32 secs    0.00 micros   92.32 secs
   sys time    0.26 secs  931.00 micros    0.26 secs

$ time hlint -j .
No hints

________________________________________________________
Executed in   18.43 secs    fish           external
   usr time  337.22 secs  704.00 micros  337.22 secs
   sys time    3.58 secs  165.00 micros    3.58 secs

@philderbeast
Copy link
Collaborator Author

consider updating the CI to use these targets if you don't want them to bitrot right away.

I had but we use haskell-actions/hlint-run for this in CI.

steps:
- uses: actions/checkout@v4
- uses: haskell-actions/hlint-setup@v2
with:
version: "3.8"
- uses: haskell-actions/hlint-run@v2
with:
path: "."
fail-on: suggestion

@ulysses4ever
Copy link
Collaborator

It can save time:

I'm not saying hlint -j vs hlint, I'm saying hlint -j vs make hlint.

I had but we use haskell-actions/hlint-run for this in CI.

Yes, this is my usual beef with GitHub actions that run things -- they foster diverging workflows locally vs. in CI, I think it's a bad design. I wish CI only used setup-actions and running stuff was done via make targets -- we'd have a single source of truth to work with.

@philderbeast
Copy link
Collaborator Author

I'm not saying hlint -j vs hlint, I'm saying hlint -j vs make hlint.

I was showing the time saving by running the recipe for the lint rule versus running hlint . naively without adding that option. Sorry I wasn't clearer about that timing comparison.

.PHONY: lint
lint: ## Run HLint
	hlint -j .

@philderbeast
Copy link
Collaborator Author

Yes, this is my usual beef with GitHub actions that run things -- they foster diverging workflows locally vs. in CI, I think it's a bad design. I wish CI only used setup-actions and running stuff was done via make targets -- we'd have a single source of truth to work with.

@ulysses4ever I agree. It can be hard to run github actions locally without hassle (having tried nektos/act).

@ulysses4ever
Copy link
Collaborator

Sorry I wasn't clearer about that timing comparison.

It's okay, maybe it was me who wasn't clear enough. Just to reiterate: I doubt that adding a make target as small as lint (only have one line) is beneficial: it saves minimal typing (you'll type make lint instead of hlint -j .) at the cost of blowing the Makefile. But again, I'm not blocking it on this ground...

@geekosaur
Copy link
Collaborator

I'm generally not sure how much profit we get from such simplistic one-line, three-four-words make rules. This saves very little but makes Makefile bigger and harder to maintain. But I'm not opposed to it. Myself, I don't really use the Makefile.

I think having a documented single source of truth for local checks is an advantage over developers having to dig to find out how we use it locally. CI using it is of course the obvious next step after that, but IIRC I was a bit worried about the way the CI actions worked and didn't want to sit down and research why they looked weird.

@ulysses4ever
Copy link
Collaborator

Yes, treating Makefile as documentation is a great idea. I agree. The only issue is that, as always with the docs, it needs active maintenance. In my experience, 50-80% of our Makefile is badly outdated. Calling make in CI could counter that, but I don't see as much energy in that direction as pumping more and more targets into the Makefile has. I think it's a path to nowhere.

@geekosaur
Copy link
Collaborator

Admittedly I've put more effort into documenting it in CONTRIBUTING.md in order to encourage its use. The counterpoint to your worry is the number of times people on yesterday's call said they push stuff up to CI and see if it fails instead of checking locally first (/cc @mpickering), which to me is wasted time. (Yes, I was doing that with the API checks, but my worry there was always irreproducibility on the dev's machine and in fact I still don't entirely trust that a dev on Windows or an M1/M2 Mac will get the same result as someone on Ubuntu 24.04.)

@ulysses4ever
Copy link
Collaborator

The counterpoint to your worry is the number of times people on yesterday's call said they push stuff up to CI and see if it fails instead of checking locally first, which to me is wasted time.

Sorry, I'm a bit lost here:

  • what is a wasted time?
  • if people "simply" use CI, how does it counter my worry that the Makefile bitrots, and inflating it only limits the hope of someone actually cleaning it up?

I'd prefer 0 bitrot in this repository. I think it's confusing to unsuspected people and therefore harmful. That's why I always bring up this point about exercising the Makefile in CI. Core devs that show up on the calls can do whatever --- they will find their way around bitrot. It's the rest 99% who I'm worried about.

@philderbeast
Copy link
Collaborator Author

Label merge+no rebase is necessary when the pull request is from an organisation.

@mergify mergify bot added the ready and waiting Mergify is waiting out the cooldown period label Nov 22, 2024
@geekosaur
Copy link
Collaborator

I think the only way CI wins for something that can easily be checked locally (which is the point of the Makefile rules) is if you can leave it to run overnight — but you can also do that in a terminal session locally.

@philderbeast
Copy link
Collaborator Author

I've been struggling to find what is failing with CI validate runs for #10546. One benefit of running locally is that it is about 30x faster.

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Nov 24, 2024
@mergify mergify bot merged commit 1103d01 into haskell:master Nov 24, 2024
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge+no rebase ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a lint rule to makefile
3 participants