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

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.

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

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.

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?

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.

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?

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?

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.

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.

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.

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!

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.

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`.

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 ============================= ```

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

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.

Labels Milestones

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

Step 1:

Step 2: