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

postprocessd: init at 0.3.0 #338795

Merged
merged 2 commits into from
Nov 6, 2024
Merged

Conversation

Luflosi
Copy link
Contributor

@Luflosi Luflosi commented Sep 1, 2024

Description of changes

Add postprocessd, a queueing Megapixels post-processor.
Also patch Megapixels to be able to use the new postprocessor if it is installed globally. See the long comment in the code for a more detailed description and a discussion of tradeoffs.

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.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Sep 1, 2024
@ofborg ofborg bot requested a review from dotlambda September 1, 2024 13:28
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Sep 1, 2024
Copy link
Member

@donovanglover donovanglover left a comment

Choose a reason for hiding this comment

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

Seems reasonable just wondering if the comment should be simplified and/or in the commit message instead.

pkgs/by-name/me/megapixels/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/po/postprocessd/package.nix Outdated Show resolved Hide resolved
@Luflosi
Copy link
Contributor Author

Luflosi commented Sep 3, 2024

Seems reasonable just wondering if the comment should be simplified and/or in the commit message instead.

Yeah, the comment is pretty long. But I felt using these global paths goes against the NixOS philosophy and is therefore controversial, so I wanted to explain everything with as much detail as possible. Feel free to tell me any concrete suggestions you may have on how to simplify it.

At first I had this text in the commit message but then I moved it into this comment because I didn't like how the code contained a patch without any explanation and users would have to search the Git history just to find an explanation why this patch exists and what it does.

pkgs/by-name/me/megapixels/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/me/megapixels/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/me/megapixels/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/me/megapixels/package.nix Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1955

@Luflosi
Copy link
Contributor Author

Luflosi commented Sep 17, 2024

I fixed a grammatical error in the comment.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1974

Copy link
Contributor

@MatthewCroughan MatthewCroughan left a comment

Choose a reason for hiding this comment

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

Let's rebase this on master to see if it still passes CI, and if so I believe we should merge this.

@Luflosi
Copy link
Contributor Author

Luflosi commented Nov 3, 2024

Rebased.

@Luflosi
Copy link
Contributor Author

Luflosi commented Nov 4, 2024

Everything except the Darwin tests ran and this program is only meant for Linux.

@MatthewCroughan MatthewCroughan merged commit 7845645 into NixOS:master Nov 6, 2024
27 of 28 checks passed
@Luflosi Luflosi deleted the add/postprocessd branch November 6, 2024 14:57
@Luflosi
Copy link
Contributor Author

Luflosi commented Nov 6, 2024

Thanks!

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: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants