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

treewide: fix runHook spelling mistakes #324603

Merged
merged 1 commit into from
Jul 7, 2024
Merged

Conversation

Aleksanaa
Copy link
Member

@Aleksanaa Aleksanaa commented Jul 4, 2024

Description of changes

Found by #324444. When you find a cockroach under the light, there may be countless cockroaches in the dark.

Even if you do not define these pre and post, as long as you define phase yourself, you should add them. This is for convenience in overriding.

We should probably enforce CI on this.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@Aleksanaa Aleksanaa changed the title treewide: fix all wrong or missing runHook {pre,post}{Build,Install} treewide: fix wrong or missing runHook {pre,post}{Build,Install} Jul 4, 2024
@AndersonTorres
Copy link
Member

This was/is one of motivations of using modules to define packages.

@Aleksanaa Aleksanaa changed the base branch from master to staging July 5, 2024 04:15
@Aleksanaa Aleksanaa changed the base branch from staging to master July 5, 2024 04:19
@Aleksanaa Aleksanaa changed the title treewide: fix wrong or missing runHook {pre,post}{Build,Install} treewide: fix runHook spelling mistakes Jul 5, 2024
@Aleksanaa
Copy link
Member Author

For others I plan to add them all and enforce CI on it.

@Aleksanaa
Copy link
Member Author

Why this? f4fb1f5dd286b25689ea7a87bd284cf6e87171d3..b2c8ae28de812c719e91bd074c398e7beb070e40 (compare)

Because this problem is wider than I expected. Many packages aren't just lacking one of runHook, but most are lacking both. I need to come up with a better way to do full treewide changes.

@drupol
Copy link
Contributor

drupol commented Jul 5, 2024

But these changes were valid, why not leaving them in?

@Aleksanaa
Copy link
Member Author

But these changes were valid, why not leaving them in?

I don't want to let others go into staging; they are more likely to be fixed in other places and thus causing merge conflicts.

@AndersonTorres
Copy link
Member

We should probably enforce CI on this.

This remembers a concept the software engineers call smell. Such a repetition of boilerplate code indicates some sneaky problem happening.

@Aleksanaa
Copy link
Member Author

This remembers a concept the software engineers call smell. Such a repetition of boilerplate code indicates some sneaky problem happening.

This repo was built by thousands of people. Wherever there is an opportunity for silent mistakes, there are dozens to hundreds of them. The best way is to set boundaries and raise the lower limit.

@AndersonTorres
Copy link
Member

About CI, a long time ago rmcgibbo created a bot that looked at a PR and ran nixpkgs-hammering over it.
And one of the diagnostics of nixpkgs-hammering was finding runHooks.

Maybe the idea can be resurrected!

@Aleksanaa Aleksanaa merged commit 4454279 into NixOS:master Jul 7, 2024
24 checks passed
@Aleksanaa Aleksanaa deleted the runHook-fsck branch July 7, 2024 14:11
@pbsds
Copy link
Member

pbsds commented Jul 7, 2024

About CI, a long time ago rmcgibbo created a bot that looked at a PR and ran nixpkgs-hammering over it. And one of the diagnostics of nixpkgs-hammering was finding runHooks.

Maybe the idea can be resurrected!

I have some stale code somewhere where i ran nixpkgs-hammer on all of nixpkgs.
https://pvv.ntnu.no/~pederbs/tmp/merged-issues-report-nixpkgs/0396d3b0fb7f/report-hammering.html

It also checked all links to github issues, pull requests and commits to check if it it has been merged and tagged upstream.
https://pvv.ntnu.no/~pederbs/tmp/merged-issues-report-nixpkgs/0396d3b0fb7f/report-upstream.html

I need to revive that at some point, but i've been swamped recently with research work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants