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

Jump to bottom

rapid-photo-downloader: 0.9.13 -> 0.9.14 #59347

Merged
merged 1 commit into from Apr 15, 2019
Merged

rapid-photo-downloader: 0.9.13 -> 0.9.14 #59347

merged 1 commit into from Apr 15, 2019

Conversation

callahad
Copy link
Member

@callahad callahad commented Apr 12, 2019 •
edited

This updates Rapid Photo Downloader to 0.9.14 and fixes #56815

Motivation for this change
  • Version bump
  • Actually make it work:
    • Does not require exiftool to be present in the user environment
    • Can now successfully find libmediainfo
    • Can now successfully generate video thumbnails

Known issues:

  • Support for RAW images is not fully functional.

    Rapid Photo Downloader uses Rawkit bindings to libraw.

    Rawkit is unmaintained, and its most recent release does not support libraw 0.19. The nixpkgs collection has included 0.19 since mid-2018.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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 nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@callahad
Copy link
Member Author

@jfrankenau, as maintainer of rapid-photo-downloader, I'd appreciate your review.

@dotlambda, you helped out with unblocking #56815, your review would also be appreciated.

@grahamc
Copy link
Member

grahamc commented Apr 12, 2019 •
edited

@GrahamcOfBorg build rapid-photo-downloader

@callahad callahad force-pushed the rapid-photo-downloader branch 2 times, most recently from 4013be5 to a673f6f Compare April 13, 2019 21:15
dotlambda
dotlambda requested changes Apr 14, 2019
View reviewed changes
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.

I'm seeing

Ran 0 tests in 0.001s

Was this the case before? We should set doCheck = false and add a comment with the reason if there are no tests.

@callahad
Copy link
Member Author

The checkPhase definitely does something, because the log output is different:

--- nocheck.txt 2019-04-14 19:37:14.927437916 +0100
+++ check.txt   2019-04-14 19:37:18.031466371 +0100
@@ -538,4 +538,33 @@
 wrapping `/nix/store/...-rapid-photo-downloader-0.9.14/bin/analyze-pv-structure'...
 Rewriting #!/nix/store/lmwk7mg8y79m3izdxdlckdn385x7jgl7-python3-3.7.3/bin/python3.7 to #!/nix/store/lmwk7mg8y79m3izdxdlckdn385x7jgl7-python3-3.7.3
 wrapping `/nix/store/...-rapid-photo-downloader-0.9.14/bin/rapid-photo-downloader'...
+running install tests
+running test
+running egg_info
+writing rapid_photo_downloader.egg-info/PKG-INFO
+writing dependency_links to rapid_photo_downloader.egg-info/dependency_links.txt
+writing entry points to rapid_photo_downloader.egg-info/entry_points.txt
+writing requirements to rapid_photo_downloader.egg-info/requires.txt
+writing top-level names to rapid_photo_downloader.egg-info/top_level.txt
+reading manifest file 'rapid_photo_downloader.egg-info/SOURCES.txt'
+reading manifest template 'MANIFEST.in'
+warning: no files found matching '*.txt' under directory 'raphodo'
+warning: no previously-included files matching '*~' found anywhere in distribution
+warning: no previously-included files matching '*.pyc' found anywhere in distribution
+warning: no previously-included files matching '*~1~' found anywhere in distribution
+warning: no previously-included files matching '*~2~' found anywhere in distribution
+warning: no previously-included files matching '*bak' found anywhere in distribution
+warning: no previously-included files found matching 'raphodo/test.py'
+warning: no previously-included files found matching 'raphodo/test2.py'
+no previously-included directories found matching 'tasks'
+no previously-included directories found matching 'raphodo/tests'
+no previously-included directories found matching 'debian'
+no previously-included directories found matching 'images'
+writing manifest file 'rapid_photo_downloader.egg-info/SOURCES.txt'
+running build_ext
+
+----------------------------------------------------------------------
+Ran 0 tests in 0.000s
+
+OK
 postPatchMkspecs

...and parts fail without exiftool in checkInputs:

--- check.txt   2019-04-14 19:37:18.031466371 +0100
+++ check-without-exiftool.txt  2019-04-14 19:38:00.453815232 +0100
@@ -562,6 +562,7 @@
 no previously-included directories found matching 'images'
 writing manifest file 'rapid_photo_downloader.egg-info/SOURCES.txt'
 running build_ext
+ERROR:root:Unable to compare ExifTool version number:
 
 ----------------------------------------------------------------------
 Ran 0 tests in 0.000s

But as far as I can tell, there aren't any actual unit tests in the upstream source. The program does contain a bunch of doctests, but they don't appear to run by default, and at least one has a comment stating that it requires actual hardware plugged in before it will pass.

...that said, I'm not seeing a meaningful difference in build time with and without checkPhase, so I'd be inclined to leave it as is should the author ever decide to add a proper test suite. Is there a compelling reason to disable it instead?

@dotlambda
Copy link
Member

But as far as I can tell, there aren't any actual unit tests in the upstream source. The program does contain a bunch of doctests, but they don't appear to run by default, and at least one has a comment stating that it requires actual hardware plugged in before it will pass.

Please run these doctests in a checkPhase.

@callahad
Copy link
Member Author

callahad commented Apr 15, 2019 •
edited

Please run these doctests in a checkPhase.

I don't think that makes sense. If the author intended for them to run regularly, they'd be executed as part of setup.py test, but they aren't. They also have things like this comment in raphodo/devices.py:

To run the doctests, ensure at least one camera is plugged in but not mounted!

...which then goes on to test paths that are hardcoded to match the author's environment (/media/damon/), as well as his Canon camera (EOS_DIGITAL).

Making these tests run and pass in the general case would require nontrivial patches against upstream.

@dotlambda
Copy link
Member

Alright, then please set doCheck = false and mention the requirement of a camera as reason.

@callahad
rapid-photo-downloader: 0.9.13 -> 0.9.14
92c331f 
This updates Rapid Photo Downloader to 0.9.14 and fixes NixOS#56815

Support for RAW images is not currently functioning; the rawkit bindings
are unmaintained and only support libraw <= 0.18, but nixpkgs ships 0.19
@callahad
Copy link
Member Author

Done.

I still feel bad about disabling the check phase altogether as it adds trivial overhead, but if you think it's better this way, there it is. :)

@dotlambda dotlambda merged commit 71727f3 into NixOS:master Apr 15, 2019
@dotlambda
Copy link
Member

Btw, if someone is interested in RAW support, they should make a PR pinning rawkit to 0.18 using packageOverrides.

@callahad callahad deleted the rapid-photo-downloader branch April 23, 2019 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotlambda dotlambda dotlambda requested changes

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

Successfully merging this pull request may close these issues.

rapid-photo-downloader / rawkit: fails to build
5 participants
@callahad @grahamc @dotlambda @GrahamcOfBorg @markuskowa