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

upscaler: init at 1.4.0 #356497

Merged
merged 3 commits into from
Nov 20, 2024
Merged

upscaler: init at 1.4.0 #356497

merged 3 commits into from
Nov 20, 2024

Conversation

LordGrimmauld
Copy link
Contributor

@LordGrimmauld LordGrimmauld commented Nov 16, 2024

New packages:

The python vulkan api bindings might or might not work on darwin systems too, I do not have any way to check.
This module can be tested individually against the example program using nix-shell -E 'with import /path/to/local/nixpkgs {}; mkShell { buildInputs = [ (python312.withPackages(ps: with ps; [ vulkan ])) ]; }' --run python3 /path/to/example/file

upscayl-ncnn is a fork of realesrgan-ncnn-vulkan which is already packaged for nix. Therefore, the package definition is very similar. I assumed the fork will run on darwin systems too, like upstream does. I do not have any way to test however. The patches had to be adapted to apply cleanly on the fork. Upscayl-ncnn is not yet part of nixpkgs because upscayl nixpkg is a repackaged AppImage, thus not building the backend from source.
This can be tested individually by upscayl-bin -i INPUT.png -o OUTPUT.png

Upscaler is a regular GUI app with desktop entry and everything. It will depend on the other two packages and thus can't really be tested individually. It should just work. Tested on nixos x86_64-linux.

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@LordGrimmauld LordGrimmauld changed the title Gnome upscaler upscaler: init at 1.4.0 Nov 16, 2024
@nix-owners nix-owners bot requested a review from natsukium November 16, 2024 15:43
@Aleksanaa
Copy link
Member

Hello, I have opened #327919 before, but it's certainly better if you can do it.

@LordGrimmauld
Copy link
Contributor Author

Oh crap, totally missed that, probably because i was looking for gnome-upscaler instead of upscaler.
I did however manage to fix the black image issue in this (upscayl-ncnn was missing models).
I did not intend to supersede your work, sorry for that! I suppose I'll adopt the feedback you got on your version if you are fine with that, but i'd also be happy just reviewing your version.

Copy link
Contributor

@nyabinary nyabinary left a comment

Choose a reason for hiding this comment

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

Some comments :3

pkgs/by-name/up/upscayl-ncnn/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/up/upscayl-ncnn/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/up/upscayl-ncnn/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/up/upscayl-ncnn/package.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/vulkan/default.nix Outdated Show resolved Hide resolved
pkgs/by-name/up/upscayl-ncnn/package.nix Outdated Show resolved Hide resolved
@LordGrimmauld LordGrimmauld force-pushed the gnome-upscaler branch 2 times, most recently from c9bba5b to 80d5bb1 Compare November 16, 2024 17:06

# Load SDK
-_lib_names = ('libvulkan.so.1', 'vulkan-1.dll', 'libvulkan.dylib')
+_lib_names = ('@vulkan_loader@/lib/libvulkan.so.1', 'vulkan-1.dll', 'libvulkan.dylib')
Copy link
Member

Choose a reason for hiding this comment

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

My solution is probably better as this does not address compatibility issues it may have on macOS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe. Ideally there'd be a way to test this stuff on darwin. I might have a poke at setting up a hackintosh to test.

pkgs/by-name/up/upscaler/package.nix Show resolved Hide resolved
pkgs/by-name/up/upscaler/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/up/upscaler/package.nix Show resolved Hide resolved
pkgs/by-name/up/upscaler/package.nix Show resolved Hide resolved
pkgs/by-name/up/upscaler/package.nix Show resolved Hide resolved
pkgs/by-name/up/upscayl-ncnn/package.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/vulkan/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/vulkan/default.nix Outdated Show resolved Hide resolved
pkgs/by-name/up/upscayl-ncnn/package.nix Outdated Show resolved Hide resolved
description = "Upscale and enhance images";
homepage = "https://tesk.page/upscaler";
license = lib.licenses.gpl3Only;
maintainers = with lib.maintainers; [ grimmauld ];
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
maintainers = with lib.maintainers; [ grimmauld ];
maintainers = with lib.maintainers; [ grimmauld getchoo ];

And feel free to add me here as well :)

Copy link
Member

Choose a reason for hiding this comment

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

(add me as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the deps too, or just upscaler?

Copy link
Member

Choose a reason for hiding this comment

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

I'd be fine with the deps too

@LordGrimmauld LordGrimmauld force-pushed the gnome-upscaler branch 2 times, most recently from 9d99164 to fa560b7 Compare November 17, 2024 14:58
@getchoo getchoo self-requested a review November 17, 2024 20:42
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Nov 18, 2024
@ofborg ofborg bot requested a review from Aleksanaa November 18, 2024 21:17
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 labels Nov 18, 2024
repo = "upscayl-ncnn";
rev = "refs/tags/${finalAttrs.version}";
hash = "sha256-rGnjL+sU5x3VXHnvuYXVdxGmHdj9eBkIZK3CwL89lN0=";
fetchSubmodules = true;
Copy link
Member

Choose a reason for hiding this comment

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

This is failing in Ofborg as the submodules use SSH, but we should actually be able to drop this as we're using the system equivalents

Suggested change
fetchSubmodules = true;

Copy link
Member

@getchoo getchoo left a comment

Choose a reason for hiding this comment

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

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 356497


aarch64-linux

✅ 6 packages built:
  • python311Packages.vulkan
  • python311Packages.vulkan.dist
  • python312Packages.vulkan
  • python312Packages.vulkan.dist
  • upscaler
  • upscayl-ncnn

@getchoo getchoo added 12.approvals: 1 This PR was reviewed and approved by one reputable person backport release-24.11 Backport PR automatically labels Nov 19, 2024
@ofborg ofborg bot requested a review from getchoo November 20, 2024 00:54
@Aleksanaa Aleksanaa merged commit d0df775 into NixOS:master Nov 20, 2024
29 of 30 checks passed
Copy link
Contributor

Successfully created backport PR for release-24.11:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: python 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person backport release-24.11 Backport PR automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants