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

stackblur-go: init at 1.1.0 #309117

Merged
merged 1 commit into from
Nov 23, 2024
Merged

stackblur-go: init at 1.1.0 #309117

merged 1 commit into from
Nov 23, 2024

Conversation

sodiboo
Copy link
Contributor

@sodiboo sodiboo commented May 4, 2024

Description of changes

Adds https://github.com/esimov/stackblur-go.

This is a CLI program that implements the Stackblur algorithm.

I was gonna use it for a low-latency blur thing on my desktop, but benchmarking showed that it's not a significant improvement, so i don't actually use this? But i did package it for nixpkgs to test this. Generated mostly with nix-init and the default exe name is cmd? I chose stackblur as a binary name (over stackblur-go) because that's what's shown in their README, and it doesn't coincide with anything else in nixpkgs.

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.05 Release Notes (or backporting 23.05 and 23.11 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.

Copy link
Member

@momeemt momeemt left a comment

Choose a reason for hiding this comment

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

Thank you for your contributions! I left the comment about with.lib. I would be grateful if you could confirm this.

Comment on lines 25 to 35
meta = with lib; {
description = "A fast, almost Gaussian Blur implementation in Go";
homepage = "https://github.com/esimov/stackblur-go";
license = licenses.mit;
maintainers = with maintainers; [ sodiboo ];
mainProgram = "stackblur";
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
meta = with lib; {
description = "A fast, almost Gaussian Blur implementation in Go";
homepage = "https://github.com/esimov/stackblur-go";
license = licenses.mit;
maintainers = with maintainers; [ sodiboo ];
mainProgram = "stackblur";
};
meta = {
description = "A fast, almost Gaussian Blur implementation in Go";
homepage = "https://github.com/esimov/stackblur-go";
license = lib.licenses.mit;
maintainers = with lib.maintainers; [ sodiboo ];
mainProgram = "stackblur";
};

The use of with lib; is discouraged. ref: #293767
You can put lib.licenses.mit in meta.license, and others too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, i was vaguely aware of this being discouraged, but i had understood it as though nixpkgs encourages meta = with lib;, since i've seen a lot of packages do exactly that, including some i've worked on [1] [2].

In this case, i feel it's probably fine? it's obvious enough where licenses and maintainers comes from and there are literally no other identifiers within its scope (except sodiboo but that one is even more obvious). I could change it i guess, but it just feels like changing it for the sake of changing it?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your reply. I understand your opinion, meta in stackblur-go certainly only contains lib identifiers. However, I think meta can be extended to include other fields that can access other identifiers within its scope (e.g. meta.broken and meta.homepage, etc.), on the other hand meta.maintainers hardly contains them. Considering that other properties will be added in the future, I think using lib.licenses without with lib; contributes to long-term maintainability.
Although, to be honest, I have only recently read 293767 and started reviewing nixpkgs, so I may be misreading it, I feel the point that using with lib; for meta is too broad is a legitimate one.

Of course, more discussion is needed on whether these changes help the project, and I welcome your opinions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've implemented the suggested changes in a recent commit.

ldflags = [ "-s" "-w" ];

meta = {
description = "A fast, almost Gaussian Blur implementation in Go";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description = "A fast, almost Gaussian Blur implementation in Go";
description = "Fast, almost Gaussian Blur implementation in Go";

@@ -0,0 +1,32 @@
{ lib
, buildGoModule
Copy link
Member

Choose a reason for hiding this comment

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

Please format with the new formatter.

@sodiboo
Copy link
Contributor Author

sodiboo commented Nov 18, 2024

(these commits should probably be squash merged )

@FliegendeWurst
Copy link
Member

(these commits should probably be squash merged )

Yes, unfortunately we don't do squash merges. Please squash them yourself.

@sodiboo
Copy link
Contributor Author

sodiboo commented Nov 21, 2024

i see. the commits have been squashed manually, and should now be ready to merge

@wegank wegank added the 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people label Nov 22, 2024
@Aleksanaa Aleksanaa merged commit 71d5dab into NixOS:master Nov 23, 2024
34 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants