photoprism: 221118-e58fee0fb -> 230603-378d4746a #230373
Conversation
pkgs/servers/photoprism/default.nix
Outdated
| --set PHOTOPRISM_IMAGEMAGICK_BIN ${imagemagick}/bin/convert \ | ||
| --set PHOTOPRISM_RSVGCONVERT_BIN ${librsvg}/bin/rsvg-convert \ |
There was a problem hiding this comment.
I'm not entirely sold that this should be part of the package update.
1.) Even if we decide to always add the dependencies imagemagick and librsvg this could be done in a separate PR - or maybe in a separate commit within this PR?
2.) I'm not entirely sold if it is really necessary to add those dependencies. As far as I can tell those are only used for converting vector graphics which I'd imagine is a rather niche application of photoprism.
Anyway, thank you for taking the time to update the package, appreciated! 👍
There was a problem hiding this comment.
The functionality for both was newly introduced within this release.
I agree that the svg converter is optional, especially since it is a non community-edition feature. Imagemagic, however, is also used to fix invalid jpgs that the golang library has trouble with (photoprism/photoprism#2463). I feel that this is closer to a core functionality that should be enabled by default. In my personal library this affects quite a few files.
My suggestion would be:
- Keep only imagemagic in this PR
- Create a separate PR/discussion on how to handle such optional dependencies. I think similar concerns can be raised for rawtherapee, which is quite heavy and (as far as i understand it) primarily used as a fallback raw converter if darktable fails. (We could either parameterize the package, or leave the binary configuration to the nixos module which can dynamically include the tools based on the settings)
There was a problem hiding this comment.
Wasn't aware that imagemagick was used this way 👍 Your suggestions sounds good, I guess I'd prefer if the module handles those runtime dependencies instead of a parameterized package. But this is for another PR :)
gregod
force-pushed
the
photoprism-230506-9de9a3540
branch
from
927ddde
to
a170401
Compare
gregod
force-pushed
the
photoprism-230506-9de9a3540
branch
from
a170401
to
804bb25
Compare
|
This new minor version sure looks shiny. Guess we should update this PR for that. |
gregod
changed the title
|
Any blockages for this PR? Also, great job and thank you for your time! |
gregod
changed the title
Not from my point of view. I'm wondering if I can do something as the package maintainer to speed up the merge process ? |
Description of changes
major: 230502-c405f6eff: https://github.com/photoprism/photoprism/releases/tag/230502-c405f6eff
minor: 230504-cbf48798c: https://github.com/photoprism/photoprism/releases/tag/230504-cbf48798c
minor: 230506-9de9a3540: https://github.com/photoprism/photoprism/releases/tag/230506-9de9a3540
minor: 230513-0b780defb: https://github.com/photoprism/photoprism/releases/tag/230513-0b780defb
minor: 230603-378d4746a: https://github.com/photoprism/photoprism/releases/tag/230603-378d4746a
I updated the src version, dependency hashes and added new binaries required to pkg wrapper.
As stated in #230194 i'm not sure if the tensorflow or models need to be updated. It seems to me that they haven't changed upstream.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)