Skip to content

We only need one rule for make clean. #2639

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Aug 22, 2022
Merged

We only need one rule for make clean. #2639

merged 7 commits into from
Aug 22, 2022

Conversation

fisx
Copy link
Contributor

@fisx fisx commented Aug 22, 2022

  • simplify and consolidate make rule names
  • drop hint in case of compiler error

@fisx fisx requested a review from smatting August 22, 2022 08:41
@fisx fisx temporarily deployed to cachix August 22, 2022 08:41 Inactive
@fisx fisx temporarily deployed to cachix August 22, 2022 08:41 Inactive
@fisx
Copy link
Contributor Author

fisx commented Aug 22, 2022

feel free to merge it this is green from ci and reviewer!

supersven
supersven requested changes Aug 22, 2022
View reviewed changes
@fisx fisx temporarily deployed to cachix August 22, 2022 09:07 Inactive
@fisx fisx temporarily deployed to cachix August 22, 2022 09:07 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 22, 2022
@fisx fisx temporarily deployed to cachix August 22, 2022 09:28 Inactive
@fisx fisx temporarily deployed to cachix August 22, 2022 09:28 Inactive
@fisx fisx requested a review from supersven August 22, 2022 09:28
supersven
supersven approved these changes Aug 22, 2022
View reviewed changes
Copy link
Contributor

@supersven supersven left a comment

Choose a reason for hiding this comment

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

Now, it looks good to me 👍

Thanks for taking the effort to clean this up.

akshaymankar
akshaymankar requested changes Aug 22, 2022
View reviewed changes
Copy link
Member

@akshaymankar akshaymankar left a comment

Choose a reason for hiding this comment

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

This is not a good idea, this will delete all the cache in CI and CI will take more than an hour to compile things.

@pcapriotti
Copy link
Contributor

Also, make clean in a single repository shouldn't affect the global or per-user state of cabal.

fisx added 2 commits August 22, 2022 14:51
@fisx
Keep clean intact, add full-clean with the new semantics.
207f80b 
- avoids global changes for `make clean`.
- keeps `make clean` intact for what the ci expects.
@fisx
changelog
4f6de80
@fisx fisx temporarily deployed to cachix August 22, 2022 12:54 Inactive
@fisx fisx temporarily deployed to cachix August 22, 2022 12:54 Inactive
@fisx fisx requested a review from akshaymankar August 22, 2022 12:54
@fisx
Copy link
Contributor Author

fisx commented Aug 22, 2022

Thanks @pcapriotti @akshaymankar! Is this better? Or would make clean-global be?

@elland
Copy link
Contributor

elland commented Aug 22, 2022

@fisx make nuke? make purge?

akshaymankar
akshaymankar approved these changes Aug 22, 2022
View reviewed changes
Copy link
Member

@akshaymankar akshaymankar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

make 🧹

@fisx fisx merged commit 3eb1ea0 into develop Aug 22, 2022
@fisx fisx deleted the tweak-makefile branch August 22, 2022 13:47
@zebot zebot mentioned this pull request Sep 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akshaymankar akshaymankar akshaymankar approved these changes

@supersven supersven supersven approved these changes

@smatting smatting Awaiting requested review from smatting

+1 more reviewer

@elland elland elland approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fisx @pcapriotti @elland @akshaymankar @supersven @zebot