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

ocrmypdf: init #65775

Merged
merged 6 commits into from Aug 18, 2019
Merged

ocrmypdf: init #65775

merged 6 commits into from Aug 18, 2019

Conversation

Kiwi
Copy link
Member

@Kiwi Kiwi commented Aug 2, 2019

Motivation for this change

Many people have tried to package this... most have failed. I myself have spent months working on it and a while sitting on it. This is the best I've come up with and the newest version I've been able to make work. I want to get something submitted and feedback before I try again on the newer releases. It works to the extent that I've used it (minimally). Some tests are disabled because of course python has a lot of problems especially on nixos... and they take an excruciatingly long time to run anyway. But most of them pass when you do.

Things done

Added OCRmyPDF and a few dependencies.

  • 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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

if it's just an application, then it needs to be moved out of python-packages.nix.

pkgs/development/python-modules/ocrmypdf/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/ocrmypdf/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/ocrmypdf/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/pikepdf/default.nix Outdated Show resolved Hide resolved
@Kiwi Kiwi force-pushed the ocrmypdf branch 2 times, most recently from ce3d50b to 76954bd Compare August 2, 2019 19:54
pytestrunner
python-xmp-toolkit
setuptools
] ++ runtimeDeps;
Copy link
Member

Choose a reason for hiding this comment

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

runTimeDeps is already added to propagatedBuildInputs, so no need to add here again

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed it there. I had to add it to checkInputs to make the tests work when they are enabled. Is that expected?

Copy link
Member

Choose a reason for hiding this comment

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

If those are only needed for tests, then yes. If they are needed when using this program, then they should be added to (propagated)(buildInputs).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's the former. It seems to work how it is now. The only thing I know about at this point is I haven't ever been able to get it this version to work in nix-review but it seems to work when installed. Not sure if that's an issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Action needed here?

Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Almost there!

@Kiwi Kiwi force-pushed the ocrmypdf branch 2 times, most recently from 5664a60 to 97b3923 Compare August 3, 2019 00:43
@Kiwi
Copy link
Member Author

Kiwi commented Aug 3, 2019

Almost there!

I like your optimism and thank you for all of the feedback!

@FRidh FRidh changed the title (WIP?) OCRmyPDF ocrmypdf: init Aug 13, 2019

src = fetchFromGitHub {
owner = "tmbdev";
repo = "${pname}";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
repo = "${pname}";
repo = pname;


meta = with stdenv.lib; {
description = "
Tools for manipulating and evaluating the hOCR format for representing multi-lingual OCR results by embedding them into HTML";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this shorter?


src = fetchFromGitHub {
owner = "saltstack";
repo = "${pname}";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
repo = "${pname}";
repo = pname;

# It includes the commits for the unreleased version 2.0.2 and more
# Tests pass with this version
src = fetchFromGitHub {
owner = "python-xmp-toolkit";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
owner = "python-xmp-toolkit";
owner = pname;

# Tests pass with this version
src = fetchFromGitHub {
owner = "python-xmp-toolkit";
repo = "python-xmp-toolkit";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
repo = "python-xmp-toolkit";
repo = pname;


src = fetchFromGitHub {
owner = "cgat-developers";
repo = "${pname}";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
repo = "${pname}";
repo = pname;

pytest
];

checkPhase = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work?

checkPhase = null;

preCheck = ''
  export HOME=$TMPDIR
'';

makefile = "ruffus/test/MakeFile"

checkTarget = "all"

checkFlags = [
  "PYTEST_OPTIONS='-q --disable-warnings'"
];

we don't cd because of setting makefile

pytestrunner
python-xmp-toolkit
setuptools
] ++ runtimeDeps;
Copy link
Contributor

Choose a reason for hiding this comment

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

Action needed here?

@FRidh FRidh merged commit 316a0e9 into NixOS:master Aug 18, 2019
@FRidh
Copy link
Member

FRidh commented Aug 18, 2019

I've merged it despite requesting further changes. After a week or so of making changes I think it is "good enough".

@Kiwi Kiwi deleted the ocrmypdf branch August 22, 2019 22:09
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.

None yet

3 participants