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

kcc: init at 5.5.1 #61146

Closed
wants to merge 1 commit into from
Closed

kcc: init at 5.5.1 #61146

wants to merge 1 commit into from

Conversation

dawidsowa
Copy link
Contributor

Motivation for this change

Add KindleComicConverter
https://kcc.iosphe.re

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.

JohnAZoidberg
JohnAZoidberg requested changes May 8, 2019
View reviewed changes
Copy link
Member

@JohnAZoidberg JohnAZoidberg left a comment

Choose a reason for hiding this comment

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

You can also encode the requirement for Python 3.3+ with:

disabled = !(pythonAtLeast "3.3");

pkgs/top-level/all-packages.nix Outdated
@@ -9203,6 +9203,8 @@ in

kati = callPackage ../development/tools/build-managers/kati { };

kcc = callPackage ../applications/graphics/kcc { };
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
kcc = callPackage ../applications/graphics/kcc { };
kcc = python3Packages.callPackage ../applications/graphics/kcc { };

Then you don't need to use python3Packages at all in the derivation and can take the python dependencies directly as argument.

pkgs/applications/graphics/kcc/default.nix Outdated
{ lib, python3Packages }:

python3Packages.buildPythonApplication rec {
pname = "KindleComicConverter";
Copy link
Member

Choose a reason for hiding this comment

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

Should not be different from the attribute in top-level.nix.
I'm divided on what to use here. On GitHub they use kcc but on PyPi they use this...

Copy link
Contributor

Choose a reason for hiding this comment

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

Plus we don't use uppercase in package names (https://nixos.org/nixpkgs/manual/#sec-package-naming)
So it's either kcc or kindlecomicconverter

pkgs/applications/graphics/kcc/default.nix Outdated
propagatedBuildInputs = with python3Packages; [ pillow pyqt5 psutil python-slugify raven ];

meta = {
description = "Python app to convert comic/manga files or folders to EPUB, Panel View MOBI or E-Ink optimized CBZ.";
Copy link
Member

Choose a reason for hiding this comment

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

No period at the end please: https://nixos.org/nixpkgs/manual/#sec-standard-meta-attributes

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

meta = {
description = "Python app to convert comic/manga files or folders to EPUB, Panel View MOBI or E-Ink optimized CBZ.";
homepage = https://kcc.iosphe.re;
Copy link
Member

Choose a reason for hiding this comment

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

We're likely going to deprecate unquoted URL syntax soon, when RFC 45 is approved.

Suggested change
homepage = https://kcc.iosphe.re;
homepage = "https://kcc.iosphe.re";

@jtojnar
Copy link
Contributor

jtojnar commented May 8, 2019 •
edited

We might want something like the following for cbz support:

  postFixup = ''
    wrapProgram $out/bin/kcc --prefix PATH : ${ lib.makeBinPath [ p7zip ]}
  '';

@dawidsowa
Copy link
Contributor Author

We might want something like the following for cbz support:

  postFixup = ''
    wrapProgram $out/bin/kcc --prefix PATH : ${ lib.makeBinPath [ p7zip ]}
  '';

Sorry if I ask, I'm new to nix, but why would we need these lines?
I've opened archives (with p7zip installed) without adding them.

@jtojnar
Copy link
Contributor

jtojnar commented May 9, 2019

We might want the expression to be self-contained.

@dawidsowa dawidsowa closed this Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c0bw3b c0bw3b c0bw3b left review comments

@JohnAZoidberg JohnAZoidberg JohnAZoidberg requested changes

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@dawidsowa @jtojnar @JohnAZoidberg @c0bw3b