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.

Sign up for GitHub

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

Jump to bottom

Adding photoflow #35545

Merged
merged 1 commit into from Apr 9, 2018
Merged

Adding photoflow #35545

merged 1 commit into from Apr 9, 2018
+62 −0

Conversation

MtP76
Copy link

@MtP76 MtP76 commented Feb 25, 2018

Motivation for this change

Adding new software

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

jtojnar
jtojnar requested changes Feb 25, 2018
View reviewed changes
pkgs/applications/graphics/photoflow/default.nix Outdated
meta = {
description = "A fully non-destructive photo retouching program providing a complete RAW image editing workflow";
homepage = http://aferrero2707.github.io/PhotoFlow/;
license = stdenv.lib.licenses.gpl3Plus;
Copy link
Contributor

Choose a reason for hiding this comment

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

You could factor out stdenv.lib before the attribute set using with construction.

pkgs/applications/graphics/photoflow/default.nix Outdated
sha256 = "06gsw0yhkdnhgcvnbsc5s4agm678ap10djvji8m2qssgnjn26vgs";
};

buildInputs = [ gettext glib libxml2 pkgconfig swig automake gobjectIntrospection cmake libtiff libjpeg fftw exiv2 lensfun gtkmm2 libraw lcms2 libexif vips expat pcre pugixml ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Tools used for building the software (gettext libxml2 pkgconfig swig automake gobjectIntrospection cmake) should go to nativeBuildInputs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also when using cmake, you should also add ninja.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, you should use gtkmm3 in accordance with #18559

Copy link
Author

Choose a reason for hiding this comment

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

Photoflow can be compiled with gtkmm3 but unfortunately it's not very usable.
Discussed here: https://discuss.pixls.us/t/help-needed-for-gtk3-theme/5803

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe mention it in comment, them.

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense. Anyway thanks for great review.

pkgs/applications/graphics/photoflow/default.nix Outdated

buildInputs = [ gettext glib libxml2 pkgconfig swig automake gobjectIntrospection cmake libtiff libjpeg fftw exiv2 lensfun gtkmm2 libraw lcms2 libexif vips expat pcre pugixml ];

enableParallelBuilding = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not necessary, cmake builds in parallel by default.

pkgs/applications/graphics/photoflow/default.nix Outdated

meta = {
description = "A fully non-destructive photo retouching program providing a complete RAW image editing workflow";
homepage = http://aferrero2707.github.io/PhotoFlow/;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use https here.

pkgs/applications/graphics/photoflow/default.nix Outdated
src = fetchFromGitHub {
owner = "aferrero2707";
repo = "PhotoFlow";
rev = "914561fcb1ca97de4814c4024d9a76e4b435c2af";
Copy link
Contributor

Choose a reason for hiding this comment

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

If you extract a version into a separate variable, you can use "v${version}".

Copy link
Contributor

Choose a reason for hiding this comment

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

[Reportedly](aferrero2707/PhotoFlow#175 (comment), the stable branch is more stable then the release. Could you use commit aferrero2707/PhotoFlow@-f9bbea183fa02412d1d17075955d2284eeaf8174 and name the package photoflow-unstable-03-06?

Copy link
Author

Choose a reason for hiding this comment

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

Did this. Release is quite old already so I hope this is better but for example gtk3 did more worse on this.

Copy link
Author

Choose a reason for hiding this comment

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

Should there be also year in "version number"? I set package name to photoflow-unstable-2018-03-06 and amended commit message according.

@jtojnar
Copy link
Contributor

jtojnar commented Feb 25, 2018

Also please use photoflow: init at 0.2.8 as the commit message.

@MtP76 MtP76 force-pushed the photoflow branch 2 times, most recently from 9435173 to 0b9dc78 Compare March 5, 2018 21:26
@MtP76 MtP76 requested review from edolstra and nbp as code owners March 5, 2018 21:26
@MtP76 MtP76 force-pushed the photoflow branch 3 times, most recently from af39079 to 79ecd4f Compare March 6, 2018 21:06
@jtojnar jtojnar removed request for edolstra and nbp March 6, 2018 22:03
jtojnar
jtojnar reviewed Mar 6, 2018
View reviewed changes
pkgs/applications/graphics/photoflow/default.nix
libjpeg
fftw
exiv2
lensfun
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason, photoflow builds its own version of lensfun. Could you fix it?

Copy link
Contributor

Choose a reason for hiding this comment

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

This should do it.

  cmakeFlags = [
    "-DBUNDLED_EXIV2=OFF"
    "-DBUNDLED_LENSFUN=OFF"
    "-DBUNDLED_GEXIV2=OFF"
  ];

Copy link
Author

Choose a reason for hiding this comment

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

Done

jtojnar
jtojnar reviewed Mar 6, 2018
View reviewed changes
pkgs/applications/graphics/photoflow/default.nix
@@ -0,0 +1,49 @@
{ stdenv, fetchFromGitHub, gettext, glib, libxml2, pkgconfig, swig, automake, gobjectIntrospection, cmake, ninja, libtiff, libjpeg, fftw, exiv2, lensfun, gtkmm2, libraw, lcms2, libexif, vips, expat, pcre, pugixml }:
Copy link
Contributor

Choose a reason for hiding this comment

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

Depends on https://github.com/darktable-org/rawspeed, could you package it as well and use that instead of the bundled one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, apparently it is not possible at the moment. I opened an issue aferrero2707/PhotoFlow#175

@matthewbauer
Copy link
Member

Okay to merge?

@jtojnar
Copy link
Contributor

jtojnar commented Apr 9, 2018

I guess it is.

@jtojnar jtojnar merged commit f1c8417 into NixOS:master Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar approved these changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MtP76 @jtojnar @matthewbauer @GrahamcOfBorg