josch/img2pdf
3
6
Fork 10
Code Issues 22 Pull requests 5 Projects Releases 3 Wiki Activity

Make tests work with ImageMagick 6 and 7, fixes #111 #117

Merged
josch merged 3 commits from sbraz/img2pdf:im7 into main 2021-08-21 20:53:42 +00:00
Conversation 15 Commits 3 Files changed 3 +23 -50
sbraz commented 2021-08-19 00:19:14 +00:00
Contributor
Copy link

Hi,
These three commits should make tests work with both ImageMagick versions. The commit messages detail each change.

I have noticed that #110 seems to be the same as the first commit of this PR and #111 is similar to the last commit.

I have tested my patch with both Debian Bullseye which ships ImageMagick 6.9 and Gentoo which ships ImageMagick 7.1.

Hi, These three commits should make tests work with both ImageMagick versions. The commit messages detail each change. I have noticed that #110 seems to be the same as the first commit of this PR and #111 is similar to the last commit. I have tested my patch with both Debian Bullseye which ships ImageMagick 6.9 and Gentoo which ships ImageMagick 7.1.
sbraz added 3 commits 2021-08-19 00:19:32 +00:00 
With Imagemagick 7, it returns the same thing as "format", which is
already checked.
This is no longer the case with ImageMagick 7.
josch commented 2021-08-19 04:35:13 +00:00
Owner
Copy link

And why should I reject #110 in favor of the changes of this pull request?

I'm also not going to vendor sRGB.icc. I rather either:

  • test different paths
  • let the distributions patch the path accordingly
  • drop the icc tests altogether
And why should I reject #110 in favor of the changes of this pull request? I'm also not going to vendor sRGB.icc. I rather either: - test different paths - let the distributions patch the path accordingly - drop the icc tests altogether
sbraz commented 2021-08-19 10:40:34 +00:00
Author
Contributor
Copy link

And why should I reject #110 in favor of the changes of this pull request?

Oh, that is not what I meant. I only noticed #110 after I fixed the tests. I can simply rebase once you merge it.

As for ICC tests, I can drop that change if you'd like. It's really not the biggest blocker here. It's rather easy to patch or skip downstream.

Please let me know what you would like me to do and I will adapt the PR accordingly.

> And why should I reject #110 in favor of the changes of this pull request? Oh, that is not what I meant. I only noticed #110 after I fixed the tests. I can simply rebase once you merge it. As for ICC tests, I can drop that change if you'd like. It's really not the biggest blocker here. It's rather easy to patch or skip downstream. Please let me know what you would like me to do and I will adapt the PR accordingly.
sbraz commented 2021-08-19 10:47:15 +00:00
Author
Contributor
Copy link

test different paths

If you agree, I can probably do something like this (untested pseudo-Python):

ICC_PROFILE = None
for path in ["/usr/share/path1", "/usr/share/path2"]:
    if os.path.exists(path"):
        ICC_PROFILE = path
        break
[…]
        if icc:
            if ICC_PROFILE is None:
                pytest.skip("No valid ICC profile found")

This way, we try to make tests pass for as many distros as possible and still fall back to skipping. Would that work for you?

Do you need different ICC profiles (the one from icc-profiles and the one from Ghostscript) or can we use the same for both tests like I did in my initial change?

> test different paths If you agree, I can probably do something like this (untested pseudo-Python): ```py ICC_PROFILE = None for path in ["/usr/share/path1", "/usr/share/path2"]: if os.path.exists(path"): ICC_PROFILE = path break […] if icc: if ICC_PROFILE is None: pytest.skip("No valid ICC profile found") ``` This way, we try to make tests pass for as many distros as possible and still fall back to skipping. Would that work for you? Do you need different ICC profiles (the one from icc-profiles and the one from Ghostscript) or can we use the same for both tests like I did in my initial change?
josch commented 2021-08-19 11:13:48 +00:00
Owner
Copy link

I just merged #109 and #110.

If you know under which paths srgb.icc is stored for different distros, then yes, it makes sense to try those and skip if nothing can be found.

I just merged #109 and #110. If you know under which paths srgb.icc is stored for different distros, then yes, it makes sense to try those and skip if nothing can be found.
sbraz commented 2021-08-19 11:18:17 +00:00
Author
Contributor
Copy link

Thanks, I'll look into possible paths for it. Did you see my other question?

Do you need different ICC profiles (the one from icc-profiles and the one from Ghostscript) or can we use the same for both tests like I did in my initial change?

Thanks, I'll look into possible paths for it. Did you see my other question? >Do you need different ICC profiles (the one from icc-profiles and the one from Ghostscript) or can we use the same for both tests like I did in my initial change?
josch commented 2021-08-19 11:22:04 +00:00
Owner
Copy link

Thanks, I'll look into possible paths for it. Did you see my other question?

Do you need different ICC profiles (the one from icc-profiles and the one from Ghostscript) or can we use the same for both tests like I did in my initial change?

I don't have an answer to that one. To get the answer, we will just have to run the tests and find out. The tests pass for you, right?

> Thanks, I'll look into possible paths for it. Did you see my other question? > >Do you need different ICC profiles (the one from icc-profiles and the one from Ghostscript) or can we use the same for both tests like I did in my initial change? I don't have an answer to that one. To get the answer, we will just have to run the tests and find out. The tests pass for you, right?
sbraz force-pushed im7 from 4166367318 to 2c287a6e98 2021-08-19 11:39:59 +00:00 Compare
sbraz commented 2021-08-19 11:40:57 +00:00
Author
Contributor
Copy link

As far as I can tell, the compare function is never called with icc=True so it is effectively unused. compare_pdfimages_png is called with icc=True and either the OpenICC profile my distro ships or the Ghostscript one work.

I have pushed a new version, I think the Ghostscript detection part could use a glob since on Arch and Gentoo, the profile moves from one version to another. I will improve this later today.

As far as I can tell, the `compare` function is never called with `icc=True` so it is effectively unused. `compare_pdfimages_png` is called with `icc=True` and either the OpenICC profile my distro ships or the Ghostscript one work. I have pushed a new version, I think the Ghostscript detection part could use a glob since on Arch and Gentoo, the profile moves from one version to another. I will improve this later today.
sbraz force-pushed im7 from 2c287a6e98 to 9036ff860c 2021-08-19 11:43:32 +00:00 Compare
sbraz force-pushed im7 from 9036ff860c to b0e5f5189f 2021-08-19 12:00:00 +00:00 Compare
sbraz commented 2021-08-19 12:02:11 +00:00
Author
Contributor
Copy link

Pushed with globs that should work on a lot of well-known distros. I've removed the line that installs ICC packages from the Travis CI file since we'll only rely on Ghostscript's profile.

I have fixed CI URLs in the README in a separate commit, feel free to cherry-pick it separately from this PR.

Pushed with globs that should work on a lot of well-known distros. I've removed the line that installs ICC packages from the Travis CI file since we'll only rely on Ghostscript's profile. I have fixed CI URLs in the README in a separate commit, feel free to cherry-pick it separately from this PR.
sbraz force-pushed im7 from b0e5f5189f to 56c7c3988d 2021-08-19 17:55:08 +00:00 Compare
sbraz force-pushed im7 from 56c7c3988d to d60b5109e0 2021-08-19 17:56:38 +00:00 Compare
sbraz commented 2021-08-20 23:35:32 +00:00
Author
Contributor
Copy link

Hi @josch, do you agree with the changes? If so, I will backport this patch to the Gentoo package so we can have working tests again.

If not, please let me know and I'll update the PR accordingly.

Hi @josch, do you agree with the changes? If so, I will backport this patch to the Gentoo package so we can have working tests again. If not, please let me know and I'll update the PR accordingly.
josch commented 2021-08-21 05:34:33 +00:00
Owner
Copy link

Are the tests really passing for you? Or are they just skipped?

For example I don't know how you can just remove installing icc-profiles-free because those profiles are needed for tests like test_png_icc.

Also notice, that the icc profiles in icc-profiles-free and the ones shipped by ghostscript are not the same even though the files are called sRGB.icc in both cases. The profiles shipped by ghostscripts are to decode images and those shipped in icc-profiles-free are do encode the profile (get embedded into the images themselves).

ICC profiles are a big mystery to me and I haven't yet found somebody who could truly tell me whether what img2pdf does is correct or not. So as long as the tests pass, that's fine for me and I can merge your commits.

Thanks!

Are the tests really passing for you? Or are they just skipped? For example I don't know how you can just remove installing `icc-profiles-free` because those profiles are needed for tests like `test_png_icc`. Also notice, that the icc profiles in `icc-profiles-free` and the ones shipped by ghostscript are *not* the same even though the files are called `sRGB.icc` in both cases. The profiles shipped by ghostscripts are to *decode* images and those shipped in `icc-profiles-free` are do *encode* the profile (get embedded into the images themselves). ICC profiles are a big mystery to me and I haven't yet found somebody who could truly tell me whether what img2pdf does is correct or not. So as long as the tests pass, that's fine for me and I can merge your commits. Thanks!
sbraz force-pushed im7 from d60b5109e0 to ae5bc0f200 2021-08-21 18:53:01 +00:00 Compare
sbraz force-pushed im7 from ae5bc0f200 to 635b08c321 2021-08-21 18:57:39 +00:00 Compare
sbraz commented 2021-08-21 20:25:41 +00:00
Author
Contributor
Copy link

Tests are passing but compare, the function that relied on icc-profiles-free is never called with icc=True so that part of the code is never tested.

If you could add a test that uses it to the main branch, I would be able to check whether Ghostscript's ICC profile works with it. If it doesn't, I will add logic to fetch the right ICC profile for this test instead of using Ghostscript's.

Tests are passing but `compare`, the function that relied on `icc-profiles-free` is never called with `icc=True` so that part of the code is never tested. If you could add a test that uses it to the main branch, I would be able to check whether Ghostscript's ICC profile works with it. If it doesn't, I will add logic to fetch the right ICC profile for this test instead of using Ghostscript's.
josch commented 2021-08-21 20:30:01 +00:00
Owner
Copy link

Tests are passing but compare, the function that relied on icc-profiles-free is never called with icc=True so that part of the code is never tested.

If you could add a test that uses it to the main branch, I would be able to check whether Ghostscript's ICC profile works with it. If it doesn't, I will add logic to fetch the right ICC profile for this test instead of using Ghostscript's.

I don't think you are right here. Have a look at the function test_png_icc which calls compare_ghostscript and compare_poppler with icc=True which then in turn call compare with icc=True.

> Tests are passing but `compare`, the function that relied on `icc-profiles-free` is never called with `icc=True` so that part of the code is never tested. > > If you could add a test that uses it to the main branch, I would be able to check whether Ghostscript's ICC profile works with it. If it doesn't, I will add logic to fetch the right ICC profile for this test instead of using Ghostscript's. I don't think you are right here. Have a look at the function `test_png_icc` which calls `compare_ghostscript` and `compare_poppler` with `icc=True` which then in turn call `compare` with `icc=True`.
sbraz commented 2021-08-21 20:37:35 +00:00
Author
Contributor
Copy link

Oh right 🙂 In that case, yes, tests do pass, no skips or anything else.

Here is the output from that Dockerfile, if you want to test yourself:

FROM debian:bullseye
RUN apt update
RUN apt install -y git imagemagick libtiff-tools libimage-exiftool-perl poppler-utils netpbm ghostscript mupdf-tools tox
RUN git clone --depth 1 --branch im7 https://gitlab.mister-muffin.de/sbraz/img2pdf
WORKDIR img2pdf
RUN sed -i 's/pytest -vv/& -r a/' tox.ini
RUN tox -e py39

[…]
src/img2pdf_test.py::test_png_icc[internal] PASSED                       [ 16%]
src/img2pdf_test.py::test_png_icc[pikepdf] PASSED                        [ 16%]
src/img2pdf_test.py::test_png_icc[pdfrw] PASSED                          [ 16%]
[…]
============================= 348 passed in 36.01s =============================
Oh right 🙂 In that case, yes, tests do pass, no skips or anything else. Here is the output from that Dockerfile, if you want to test yourself: ```Dockerfile FROM debian:bullseye RUN apt update RUN apt install -y git imagemagick libtiff-tools libimage-exiftool-perl poppler-utils netpbm ghostscript mupdf-tools tox RUN git clone --depth 1 --branch im7 https://gitlab.mister-muffin.de/sbraz/img2pdf WORKDIR img2pdf RUN sed -i 's/pytest -vv/& -r a/' tox.ini RUN tox -e py39 ``` ``` […] src/img2pdf_test.py::test_png_icc[internal] PASSED [ 16%] src/img2pdf_test.py::test_png_icc[pikepdf] PASSED [ 16%] src/img2pdf_test.py::test_png_icc[pdfrw] PASSED [ 16%] […] ============================= 348 passed in 36.01s ============================= ```
josch merged commit 635b08c321 into main 2021-08-21 20:53:42 +00:00
josch commented 2021-08-21 21:07:10 +00:00
Owner
Copy link

Since the tests pass for you, I have merged this. But the test_png_icc tests keep failing: https://travis-ci.com/github/josch/img2pdf

Since the tests pass for you, I have merged this. But the `test_png_icc` tests keep failing: https://travis-ci.com/github/josch/img2pdf
sbraz commented 2021-08-21 23:49:59 +00:00
Author
Contributor
Copy link

Thanks! I tested commit 55d589a548 with Ubuntu 20 to match what Travis does and it fails the ICC tests (and TIFF tests) there too so the problem isn't linked to this PR or the change of the ICC profile:

ERROR src/img2pdf_test.py::test_tiff_rgb14[internal] - AssertionError: [{'ima...
ERROR src/img2pdf_test.py::test_tiff_rgb14[pikepdf] - AssertionError: [{'imag...
ERROR src/img2pdf_test.py::test_tiff_rgb14[pdfrw] - AssertionError: [{'image'...
ERROR src/img2pdf_test.py::test_tiff_rgb12[pdfrw] - AssertionError: [{'image'...
ERROR src/img2pdf_test.py::test_tiff_rgb12[pikepdf] - AssertionError: [{'imag...
ERROR src/img2pdf_test.py::test_tiff_rgb12[internal] - AssertionError: [{'ima...
FAILED src/img2pdf_test.py::test_png_icc[pdfrw] - assert 48.4551 > 50
FAILED src/img2pdf_test.py::test_png_icc[pikepdf] - assert 48.4551 > 50
FAILED src/img2pdf_test.py::test_png_icc[internal] - assert 48.4551 > 50

I also see the TIFF failures on Arch (ImageMagick 7) with this Dockerfile, I wonder why Debian Bullseye or Gentoo don't exhibit the same problem:

FROM archlinux
RUN pacman --noconfirm -Syu
RUN pacman --noconfirm -S git imagemagick ghostscript python-tox python-setuptools perl-image-exiftool poppler netpbm
# Ugly PATH hack
RUN ln -s /usr/bin/vendor_perl/exiftool /usr/bin
RUN git clone --depth 1 --branch im7 https://gitlab.mister-muffin.de/sbraz/img2pdf
WORKDIR img2pdf
# Make tests run faster with xdist
RUN sed -iE 's/pytest -vv/& -r a -n auto/;s/pdfrw/&\n    pytest-xdist/' tox.ini
RUN tox -e py39

I guess both the color depth problem and the ICC comparison problem could use separate issues.

Thanks! I tested commit 55d589a548cd881c3fe5a94a1c7e2db6a88ae8ea with Ubuntu 20 to match what Travis does and it fails the ICC tests (and TIFF tests) there too so the problem isn't linked to this PR or the change of the ICC profile: ``` ERROR src/img2pdf_test.py::test_tiff_rgb14[internal] - AssertionError: [{'ima... ERROR src/img2pdf_test.py::test_tiff_rgb14[pikepdf] - AssertionError: [{'imag... ERROR src/img2pdf_test.py::test_tiff_rgb14[pdfrw] - AssertionError: [{'image'... ERROR src/img2pdf_test.py::test_tiff_rgb12[pdfrw] - AssertionError: [{'image'... ERROR src/img2pdf_test.py::test_tiff_rgb12[pikepdf] - AssertionError: [{'imag... ERROR src/img2pdf_test.py::test_tiff_rgb12[internal] - AssertionError: [{'ima... FAILED src/img2pdf_test.py::test_png_icc[pdfrw] - assert 48.4551 > 50 FAILED src/img2pdf_test.py::test_png_icc[pikepdf] - assert 48.4551 > 50 FAILED src/img2pdf_test.py::test_png_icc[internal] - assert 48.4551 > 50 ``` I also see the TIFF failures on Arch (ImageMagick 7) with this Dockerfile, I wonder why Debian Bullseye or Gentoo don't exhibit the same problem: ```Dockerfile FROM archlinux RUN pacman --noconfirm -Syu RUN pacman --noconfirm -S git imagemagick ghostscript python-tox python-setuptools perl-image-exiftool poppler netpbm # Ugly PATH hack RUN ln -s /usr/bin/vendor_perl/exiftool /usr/bin RUN git clone --depth 1 --branch im7 https://gitlab.mister-muffin.de/sbraz/img2pdf WORKDIR img2pdf # Make tests run faster with xdist RUN sed -iE 's/pytest -vv/& -r a -n auto/;s/pdfrw/&\n pytest-xdist/' tox.ini RUN tox -e py39 ``` I guess both the color depth problem and the ICC comparison problem could use separate issues.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

Something is not working

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
forwarded

not an img2pdf bug but in a dependency

  
help wanted

Need some help

  
invalid

Something is wrong

  
question

More information is needed

  
wontfix

This won't be fixed

No labels
bug
duplicate
enhancement
forwarded
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: josch/img2pdf#117
No description provided.
Delete branch "sbraz/img2pdf:im7"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?