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

Update Pillow/Willow dependencies to Wagtail 5.0.x to allow installing Pillow 10.0.1+ #10989

Merged
merged 2 commits into from
Oct 4, 2023

Conversation

zerolab
Copy link
Contributor

@zerolab zerolab commented Oct 4, 2023

Similar to #10951 / #10955

Kudos to @tomkins for the heads up

@zerolab
Copy link
Contributor Author

zerolab commented Oct 4, 2023

RTD failed with

The configuration file required to build documentation is missing from your project. Add a configuration file to your project to make it build successfully. Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html

Will have a look

setup.py Outdated
@@ -27,10 +27,10 @@
"djangorestframework>=3.11.1,<4.0",
"django-filter>=2.2,<23",
"draftjs_exporter>=2.1.5,<3.0",
"Pillow>=9.1.0,<10.0.0",
"Pillow>=6.0.0,<11.0.0", # update together with willow
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you intend to reduce the lower bound here? This was set as 9.1.0 in #10911.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not. Copy/pasta error. Fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do wonder whether we need the Pillow line at all, since at the end of the day we are restricted by the Pillow versions that Willow supports 🤔

Copy link
Member

Choose a reason for hiding this comment

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

^ I've been wondering about that as well. If that's the case it's going to be so much easier to handle any security fixes coming from Pillow as we can just make a new release of Willow without a new release of Wagtail.

Copy link

Choose a reason for hiding this comment

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

When I was implementing #10911 I too wondered about this. But as I understand it from the comments, Willow can make use of either Pillow or Wand, and therefore it can’t include Pillow (or Wand) as a dependency in the packaging. But I’m not an expert in packaging, maybe there is a way of configuring constraints on optional dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went back and checked, and indeed we don't require either Pillow or Wand in Willow - https://github.com/wagtail/Willow/blob/main/pyproject.toml#L28-L31 So this was but a dream

Copy link

Choose a reason for hiding this comment

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

Maybe the answer is to add to the [project.optional-dependencies] in Willow, so that Wagtail requires Willow[Pillow,heif]>=1.6.3,<1.7 and Willow specifies Pillow>=9.1.0,<11.0.0 in that optional dependency. There would need to be a non-heif option too. Then Wagtail can remove the direct dependency on Pillow.

I'm not sure if this would introduce any breaking changes, especially to non-Wagtail uses of Willow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is actually a pretty good suggestion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zerolab zerolab force-pushed the fix/loosen-pillow-willow-deps branch from d6b7b51 to a706deb Compare October 4, 2023 10:17
@squash-labs
Copy link

squash-labs bot commented Oct 4, 2023

Manage this branch in Squash

Test this branch here: https://zerolabfixloosen-pillow-willow-bj5k7.squash.io

@thibaudcolas
Copy link
Member

thibaudcolas commented Oct 4, 2023

The CircleCI UI test failure looks legit but is unrelated to the changes here. I’ll open an issue. Edit: #10990.

Logs for ref
 FAIL  client/tests/integration/homepage.test.js
  ● Homepage › axe

    expect(received).toPassAxeTests(expected)

    Expected page to pass Axe accessibility tests.
    Violations found:
    Rule: "color-contrast" (Elements must have sufficient color contrast)
    Help: https://dequeuniversity.com/rules/axe/4.3/color-contrast?application=axe-puppeteer
    Affected Nodes:
      a[data-w-upgrade-target="link"]
        Fix ANY of the following:
        - Element has insufficient color contrast of 4.25 (foreground color: #007d7e, background color: #faecd5, font size: 10.2pt (13.6px), font weight: normal). Expected contrast ratio of 4.5:1

@gasman gasman merged commit da2cf66 into wagtail:stable/5.0.x Oct 4, 2023
17 of 19 checks passed
gasman added a commit that referenced this pull request Oct 4, 2023
gasman added a commit that referenced this pull request Oct 4, 2023
gasman added a commit that referenced this pull request Oct 4, 2023
@zerolab zerolab deleted the fix/loosen-pillow-willow-deps branch October 4, 2023 13:44
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.

None yet

6 participants