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

Rebase tests with new fonttools #863

Merged
merged 4 commits into from
Jul 25, 2023
Merged

Conversation

Tolker-KU
Copy link

Bumps fonttools to newest version and regenerate reference files for failing tests.

Checklist:

  • The GitHub pipeline is OK (green),
    meaning that both pylint (static code analyzer) and black (code formatter) are happy with the changes of this PR.

  • A unit test is covering the code added / modified by this PR

  • This PR is ready to be merged

  • In case of a new feature, docstrings have been added, with also some documentation in the docs/ folder

  • A mention of the change is present in CHANGELOG.md

By submitting this pull request, I confirm that my contribution is made under the terms of the GNU LGPL 3.0 license.

@Tolker-KU Tolker-KU requested a review from Lucas-C as a code owner July 24, 2023 17:07
setup.cfg Outdated
@@ -43,7 +43,7 @@ install_requires =
Pillow>=6.2.2,!=9.2.* # minimum version tested there: https://github.com/PyFPDF/fpdf2/actions/runs/2295868575
# Version 9.2.0 is excluded due to DoS vulnerability with TIFF images: https://github.com/PyFPDF/fpdf2/issues/628
# Version exclusion explained here: https://devpress.csdn.net/python/630462c0c67703293080c302.html
fonttools>=4.34.0
Copy link
Member

@Lucas-C Lucas-C Jul 24, 2023

Choose a reason for hiding this comment

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

I agree that we should support fonttools>=4.41.1,
but why should we set this as the minimal version?

fpdf2 can be used with older versions of fonttools,
so this change will restrain fpdf2 compatibility with fonttools,
which could be annoying for end-users that need to use fpdf2 with older versions of fonttools.

I'd prefer to only define there in setup.cfg the very absolute minimal versions of our dependencies that fpdf2 can work with.

Also, do you know what change in a fonttools release caused those differences in reference PDF files?
Could it be this, mentioned in the release notes for v4.39.0?

[feaLib] Fixed handling of ignore statements with unmarked glyphs to match makeotf behavior, which assumes the first glyph is marked

Copy link
Author

Choose a reason for hiding this comment

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

Some thoughts here: #863 (comment)

@andersonhc
Copy link
Collaborator

I tested all the latest fonttools versions against our current reference files:
4.38.0 = OK
4.39.0 = OK
4.39.2 = OK
4.39.3 = OK
4.39.4 = OK
4.40.0 = OK
4.41.0 = Fail
4.41.1 = Fail

All the versions are producing valid and visually similar PDF files so we can say fpdf2 is compatible with all those versions, however some detail (like a timestamp) changed after fonttools 4.41.0 that is causing pytest to fail.

@Tolker-KU
Copy link
Author

Tolker-KU commented Jul 24, 2023

I've done some digging. Bytestreams for fonts change from version 4.40.0 to 4.41.0. At the moment I suspect that it might be this commit fonttools/fonttools@4660b8c, but I'm not quite sure.

So our problem is that fonttools produces different bytestreams from different versions even though PDFs probably always will be render the same way - at least to the human eye. Also the problem is only seen with a few fonts. So in order to have tests pass reference pdfs and the fonttools version must be in sync. This can be achieved either by pinning fonttools (and regenerating reference files if pinning to the new version) or by having different reference PDFs for different versions of fonttols, but that is probably not something we want.

@Lucas-C
Copy link
Member

Lucas-C commented Jul 24, 2023

I've done some digging. Bytestreams for fonts change from version 4.40.0 to 4.41.0. At the moment I suspect that it might be this commit fonttools/fonttools@4660b8c, but I'm not quite sure.

Thank you for investigating 👍

This can be achieved either by pinning fonttools (and regenerating reference files if pinning to the new version) or by having different reference PDFs for different versions of fonttols, but that is probably not something we want.

Actually we could update the relevant PDF reference files, as you did in this PR, WITHOUT altering the pin on fonttools version in setup.cfg. And that is the option I would prefer to adopt 😊

Could you please just try to update this PR and revert the change you made in setup.cfg? I think the pipeline should still pass.

The latest pipeline execution on master has just failed due to the same 4 tests:

  • test/fonts/test_font_fallback.py::test_fallback_font_with_overriden_get_fallback_font
  • test/fonts/test_charmap.py::test_charmap_first_999_chars[Quicksand-Regular.otf]
  • test/fonts/test_add_font.py::test_add_font_otf
  • test/encryption/test_encryption.py::test_encrypt_font

@Tolker-KU
Copy link
Author

I've reverted the changes to setup.cfg and CHANGELOG.md. It works for users, but developers must be aware that tests will fail if they are running a too old version of fonttools

@Tolker-KU Tolker-KU changed the title Bump fonttools and rebase tests Rebase tests with new fonttools Jul 24, 2023
@Lucas-C
Copy link
Member

Lucas-C commented Jul 24, 2023

I've reverted the changes to setup.cfg and CHANGELOG.md. It works for users, but developers must be aware that tests will fail if they are running a too old version of fonttools

Agreed, perfect! 👍

If that really bothers you, and you think that's useful, you can still add fonttools>=4.41.1 to test/requirements.txt

@Tolker-KU
Copy link
Author

I'm fine with the changes as is. We've this thread for reference if the problem ever comes up :)

@Lucas-C
Copy link
Member

Lucas-C commented Jul 24, 2023

I'm fine with the changes as is. We've this thread for reference if the problem ever comes up :)

Alright!

Then I'll merge this tomorrow in my time zone, when the pipeline will be finished and ✅

@Tolker-KU
Copy link
Author

Python 3.7 got a too old version of fonttools making the tests fail :( https://github.com/PyFPDF/fpdf2/actions/runs/5650397495/job/15306727703?pr=863#step:6:114

Tests passed on Python 3.11

@andersonhc
Copy link
Collaborator

Python 3.7 got a too old version of fonttools making the tests fail :( https://github.com/PyFPDF/fpdf2/actions/runs/5650397495/job/15306727703?pr=863#step:6:114

Tests passed on Python 3.11

Fonttools dropped support to python 3.7, that's why it's not downloading the new version on the 3.7 ci

@Lucas-C
Copy link
Member

Lucas-C commented Jul 24, 2023

Fonttools dropped support to python 3.7, that's why it's not downloading the new version on the 3.7 ci

Damn. You're right, it's mentioned in the release notes for v4.39.0.

What are our options?

  • also drop support for Python 3.7
  • pin fonttools==4.38.0
  • annotate those 4 unit tests so that they do not run under Python 3.7, and use a dependency conditional to the python version in test/requirements.txt: fonttools>=4.41.1; python_version>'3.7'

I'd say the 3rd option is probably the best.
We already use some skipif logic: https://github.com/PyFPDF/fpdf2/blob/master/test/image/image_types/test_insert_images.py#L24

@Tolker-KU
Copy link
Author

Tolker-KU commented Jul 25, 2023

I've implemented the 3rd option. But we might want to consider dropping support for Python 3.7 soon-ish. 3.7 is EOL and will not receive security updates anymore. https://peps.python.org/pep-0537/#lifespan

test/requirements.txt Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

❗ No coverage uploaded for pull request base (master@e9abc00). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head 47605db differs from pull request most recent head a84f6f7. Consider uploading reports for the commit a84f6f7 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #863   +/-   ##
=========================================
  Coverage          ?   93.39%           
=========================================
  Files             ?       27           
  Lines             ?     7386           
  Branches          ?     1333           
=========================================
  Hits              ?     6898           
  Misses            ?      307           
  Partials          ?      181           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Lucas-C Lucas-C merged commit f2fe807 into py-pdf:master Jul 25, 2023
@Lucas-C
Copy link
Member

Lucas-C commented Jul 25, 2023

Thank you for your time and for this contribution to fpdf2 @Tolker-KU!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants