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

More external gd-2.3.3 test fixes #11264

Closed
wants to merge 4 commits into from

Conversation

orlitzky
Copy link
Contributor

Support for the "gd" image format was removed from gd-2.3.3, breaking several of PHP's tests when a new enough external gd is used. Here I've addressed a few more failures that required some investigation.

This test uses the imagegd2() function to check that

  libgd/libgd#289

is fixed. When an external gd without support for the "gd" format is
used, no error is thrown, but a nonsense result is printed: this is
normal. The corresponding upstream test is disabled in that situation;
it's not expected to work.

This commit skips the corresponding PHP test under the same
circumstances to fix a test failure with external gd >= 2.3.3.
This test uses the imagegd2() function to check that

  libgd/libgd#309

is fixed. When an external gd without support for the "gd" format is
used, no error is thrown, but a nonsense result is printed: this is
normal. The corresponding upstream test is disabled in that situation;
it's not expected to work.

This commit skips the corresponding PHP test under the same
circumstances to fix a test failure with external gd >= 2.3.3.
This test ensures that the third (chunk_size) parameter to imagegd2()
is respected when a fourth parameter is also given. However, when an
external gd without support for the "gd" format is used, the call to
imagegd2() does not really work at all. It doesn't fail, but it
produces an "image" with a nonsense chunk size.

To avoid failures when an external gd >= 2.3.3 is used, we skip the
test entirely in that case.
This test fails with an external gd because the test expects "Invalid"
where upstream gd says "invalid". This commit tweaks the expected
output to accept an arbitrary character in the i/I position.
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

I plan on merging your 3 PRs as a single commit to 8.1 instead of master.
Do you plan on fixing the remaining gd issues too?

@orlitzky
Copy link
Contributor Author

I plan on merging your 3 PRs as a single commit to 8.1 instead of master. Do you plan on fixing the remaining gd issues too?

They should apply to 8.2 too?

I'll look into the other issues as time permits. I did the easy ones first, so the ones that remain are getting more confusing.

@nielsdos
Copy link
Member

They should apply to 8.2 too?

Yes. The way we work is we let the PR target the lowest applicable bugfix branch (in this case 8.1) and then we merge the patch upwards to any branch that also needs the patch. So IOW I will apply it to 8.1, 8.2 & master.

I'll look into the other issues as time permits. I did the easy ones first, so the ones that remain are getting more confusing.

I see. I'll wait a bit and otherwise I'll merge the current PRs as a partial fix for the referenced issue report.

@orlitzky
Copy link
Contributor Author

Ok thanks. Do you know if any of the CI checks use an external gd? We only have two versions of gd in Gentoo, so when a test fails, it's not always clear to me whether it's my version at fault or if instead the test never worked with an external gd to begin with.

@nielsdos
Copy link
Member

As far as I can see, we always use bundled gd. So it's probably the upstream's fault if it doesn't work.

@pierrejoye
Copy link
Contributor

pierrejoye commented May 19, 2023 via email

@orlitzky
Copy link
Contributor Author

We don't tests removed GD/GD2 format. I did not check but the php related tests should skip if the format is not available. Bundled or not, each format can be disabled. Please let me know if some help is needed, happy to help :)

That's more or less what I've done. All of the other formats have an entry in gd_info() that show whether or not they're enabled, allowing the tests to be skipped if not. The GD/GD2 format, on the other hand, is never listed; so as an indirect indicator I'm using the version of gd instead. I think practically very few people will have a custom-built gd >= 2.3.3 with the support for those formats re-enabled. Gentoo is usually the most flexible distribution in that regard, and even we don't allow it.

@orlitzky
Copy link
Contributor Author

orlitzky commented Jul 6, 2023

I plan on merging your 3 PRs as a single commit to 8.1 instead of master. Do you plan on fixing the remaining gd issues too?

I think I've done all I can. The last PR was #11280. The few remaining issues will need some input from someone better acquainted with GD.

@nielsdos
Copy link
Member

nielsdos commented Jul 6, 2023

@orlitzky OK. I'll merge your PRs as a partial fix for the issue later today.

@nielsdos nielsdos closed this in 0aaad46 Jul 6, 2023
@nielsdos
Copy link
Member

nielsdos commented Jul 6, 2023

Thanks!

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