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

photoprism: init at 220302-0059f429 #170953

Closed
wants to merge 14 commits into from

Conversation

Misterio77
Copy link
Member

@Misterio77 Misterio77 commented Apr 30, 2022

Description of changes

Photoprism is a server-based application for browsing, organizing, and sharing your personal photo collection.

Most of this work was done by @newAM. I added aarch64 support and polished it up tiny bit.

I have an instance up and running on my RPi for a month now, and it's been working great.

Fixes #96043
Supersedes #99016

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.05 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@newAM newAM mentioned this pull request Apr 30, 2022
@Misterio77 Misterio77 force-pushed the init-photoprism branch 3 times, most recently from 1e41077 to 6224cfa Compare May 1, 2022 14:50
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

Did you take package-lock.json directly from upstream? Can we turn generate-dependencies.sh into an update script that also downloads the newest package-lock.json?

nixos/doc/manual/release-notes/rl-2205.section.md Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/photoprism.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/photoprism.nix Outdated Show resolved Hide resolved
@Misterio77
Copy link
Member Author

Misterio77 commented May 1, 2022

Did you take package-lock.json directly from upstream? Can we turn generate-dependencies.sh into an update script that also downloads the newest package-lock.json?

Yup, that's right. _package-lock.json is taken from upstream. I can for sure make the script also download it.

Should I somehow make the script get the version from the .nix file, or is a version bash variable on the script itself enough?

@Misterio77 Misterio77 force-pushed the init-photoprism branch 4 times, most recently from 1e58a2d to f790151 Compare May 1, 2022 18:18
@Misterio77 Misterio77 force-pushed the init-photoprism branch 4 times, most recently from 7b40b2d to 3ff79d0 Compare May 1, 2022 18:54
@dotlambda
Copy link
Member

Should I somehow make the script get the version from the .nix file, or is a version bash variable on the script itself enough?

It should either get the newest version from GitHub or use $1.

Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

Can we put libtensorflow, backend, and frontend in separate files?

Also, did you consider using nodePackages? That's what I did for deltachat-desktop and it works the following way:

  • package.json is downloaded and added to Nixpkgs
  • that file is referenced in pkgs/development/node-packages/node-packages.json
  • whenever pkgs/development/node-packages/generate.sh is run the dependencies for PhotoPrism are included in pkgs/development/node-packages/node-packages.nix
  • in pkgs/development/node-packages/default.nix you have
photoprism = super."photoprism-../../servers/photoprism".override {
  meta.broken = true; # use the top-level package instead
};
  • in pkgs/servers/photoprism/frontend.nix you use
nodejs-12_x.pkgs.photoprism.override {
  ...
}

The advantage is that it doesn't add as many lines of code to Nixpkgs, the disadvantage is that it doesn't respect package-lock.json.

pkgs/servers/photoprism/generate-dependencies.sh Outdated Show resolved Hide resolved
pkgs/servers/photoprism/backend.nix Outdated Show resolved Hide resolved
pkgs/servers/photoprism/backend.nix Outdated Show resolved Hide resolved
pkgs/servers/photoprism/frontend.nix Outdated Show resolved Hide resolved
pkgs/servers/photoprism/frontend.nix Outdated Show resolved Hide resolved
pkgs/servers/photoprism/frontend.nix Outdated Show resolved Hide resolved
pkgs/servers/photoprism/libtensorflow.nix Outdated Show resolved Hide resolved
Comment on lines 35 to 36
mkdir $out
cp -r assets $out/
Copy link
Member

@dotlambda dotlambda Jun 8, 2022

Choose a reason for hiding this comment

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

Suggested change
mkdir $out
cp -r assets $out/
install -Dt $out assets

though I'm not sure assets is the best directory name since it's very nonstandard, but it's okay for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could instead use something more standard and change PHOTOPRISM_ASSETS_PATH.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe share/photoprism?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like a good one :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need to wrap to change the environment variable?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that should work.
What's the default value for this environment variable, i.e. why did $out/assets work?

pkgs/servers/photoprism/default.nix Outdated Show resolved Hide resolved
Comment on lines +44 to +48
darktable
rawtherapee
ffmpeg
libheif
exiftool
Copy link
Member

Choose a reason for hiding this comment

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

Where are these used? Do we need to put them in $PATH using makeWrapper instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those are all runtime deps. Should we make a wrapper instead?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I don't see how buildInputs would work.

pkgs/servers/photoprism/default.nix Outdated Show resolved Hide resolved
pkgs/servers/photoprism/default.nix Outdated Show resolved Hide resolved
pkgs/servers/photoprism/default.nix Outdated Show resolved Hide resolved
@dotlambda
Copy link
Member

Sorry for the wall of nitpicky comments!

Misterio77 and others added 13 commits June 8, 2022 12:01
Co-authored-by: Robert Schütz <github@dotlambda.de>
Co-authored-by: Robert Schütz <github@dotlambda.de>
Co-authored-by: Robert Schütz <github@dotlambda.de>
Co-authored-by: Robert Schütz <github@dotlambda.de>
Co-authored-by: Robert Schütz <github@dotlambda.de>
Co-authored-by: Robert Schütz <github@dotlambda.de>
Co-authored-by: Robert Schütz <github@dotlambda.de>
Co-authored-by: Robert Schütz <github@dotlambda.de>
Co-authored-by: Robert Schütz <github@dotlambda.de>
Co-authored-by: Robert Schütz <github@dotlambda.de>
Co-authored-by: Robert Schütz <github@dotlambda.de>
Co-authored-by: Robert Schütz <github@dotlambda.de>
Co-authored-by: Robert Schütz <github@dotlambda.de>
@Misterio77
Copy link
Member Author

Sorry for the wall of nitpicky comments!

No problem at all! Thanks a lot for the very helpful comments :)

@Stunkymonkey
Copy link
Contributor

any chance this will get merged before 22.11 release? I would really like to have it available, but I am currently short on time.

@Misterio77
Copy link
Member Author

Hey, I ended up not using photoprism a lot so this PR is a little stale (but should still work!). Feel free to take it over if you're interested!

@BeneSim BeneSim mentioned this pull request Nov 5, 2022
13 tasks
@NickCao
Copy link
Member

NickCao commented Nov 8, 2022

superseded by #199674

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.

Package Photoprism
7 participants