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

ftgl: Fix builds with freetype 2.13.3 #27288

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

halostatue
Copy link
Contributor

Description

Based on research into this issue, freetype 2.13.3 changed some return types that caused current (modern?) builds to fail as seen in the linked ticket.

A patch was applied with frankheckenbach/ftgl#20 that appears to fix this, but the repo owner has not indicated a plan to cut a new release with this and other fixes since v2.4.0.

In addition to the patch files that were previously present, I have applied all other patches from v2.4.0 to current HEAD. This involved removing the existing patch files, but those changes have been fully incorporated.

Builds with +doc and +universal variants worked fine and I have tested a downstream port (gource) with this on an affected system.

Closes: https://trac.macports.org/ticket/71434

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 15.2 24C101 arm64
Xcode 16.2 16C5032a

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL in commit message?
  • checked your Portfile with port lint?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vs install?
  • tested basic functionality of all binary files?
  • checked that the Portfile's most important variants haven't been broken?

Copy link
Contributor

@ryandesign ryandesign left a comment

Choose a reason for hiding this comment

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

You seem to be making a lot of changes here that have nothing to do with the goal of fixing the build with the latest freetype.

For example, you've combined the existing patchfiles AC_ARG_WITH.patch and dylib_file.patch into a new file autoconf.patch, in the process deleting the commentary at the top that explained the purpose of those patches.

You've added a patch CMakeLists-pkgconfig.patch that makes changes to the cmake build system, but the port uses autoconf, not cmake.

You've added an empty patchfile fontdemo.patch.

You've added a patch debian.patch that changes files in the debian directory, which, not being Debian, MacPorts doesn't use.

In another patch, you added a new script ftgl-release which the developers of ftgl use to make releases. We are not the developers of ftgl so we have no use for this script.

And of course you have the workaround for the empty-named distfile being added, which I believe can be removed because I think it was caused by having an extra backslash at the end of one of the patchfile lines in the version of the code proposed in the ticket; the version of the code in this PR doesn't have that error.

I'd appreciate a more targeted PR that fixes just the problem at hand and doesn't make a bunch of unrelated unnecessary changes. Incorporating additional useful upstream fixes is ok but I would omit changes that do not affect us.

graphics/ftgl/Portfile Outdated Show resolved Hide resolved
@halostatue
Copy link
Contributor Author

You seem to be making a lot of changes here that have nothing to do with the goal of fixing the build with the latest freetype.

You are correct. I don't know anything about how this library actually works—I only need this because I need it for gource—and it seemed safer to me to pick all the changes that the upstream developer has merged since v2.4.0 was released, including the two patches that were previously applied.

While I believe that the fix in frankheckenbach/ftgl#20 is likely all that's required, I’m intentionally applying the bigger fix precisely because simply cherry-picking changes is something that only someone who has familiarity with a particular codebase should be doing.

For example, you've combined the existing patchfiles AC_ARG_WITH.patch and dylib_file.patch into a new file autoconf.patch, in the process deleting the commentary at the top that explained the purpose of those patches.

I did, and that was something that — if desired — I can reconstruct differently, since my changes were originally pulled from the GitHub .patch URLs for the commits and PRs. (The issue that caused me to collect the patches together by logical groupings turned out to be not replacing -p0 with -p1 since git patches are always built with a synthetic level.) For the reasons I described above (cherry-picking the patches seems reckless when you don't know anything about the library in question), I would want to reconstruct the patches from git format-patch v2.4.0..HEAD, which would provide the commentary explaining each of the patches applied (and would avoid the issue where fontdemo.patch was empty).

And of course you have the workaround for the empty-named distfile being added, which I believe can be removed because I think it was caused by having an extra backslash at the end of one of the patchfile lines in the version of the code proposed in the ticket; the version of the code in this PR doesn't have that error.

You are correct and I have removed that.

I'd appreciate a more targeted PR that fixes just the problem at hand and doesn't make a bunch of unrelated unnecessary changes. Incorporating additional useful upstream fixes is ok but I would omit changes that do not affect us.

I’ll be happy to rework this tomorrow to base the patches provided on git format-patch instead of the haphazard reconstruction I did, but as I said, I don't feel comfortable cherry-picking the patches. If someone (like yourself, perhaps) who is both more comfortable with this library and MacPorts in general wants to identify which ones should be skipped, here’s the list from git format-patch v2.4.0..HEAD:

0001-fix-duplicated-entry-in-doc.patch
0002-debian-watch-debian-rules-synced-from-Debian.patch
0003-ftgl-release-new-script.patch
0004-debian-changelog-debian-rules-synced-from-Debian.patch
0005-src-CMakeLists.txt-remove-FTLibrary.h-from-libftgl_l.patch
0006-src-FTFont-FTBufferFont.cpp-src-FTFont-FTTextureFont.patch
0007-Install-pkgconfig-when-building-with-cmake.patch
0008-Use-AC_ARG_WITH-correctly-so-that-the-advertised-con.patch
0009-Use-correct-syntax-for-the-dylib_file-flag.patch
0010-demo-FTGLDemo.cpp-demo-FTGLMFontDemo.cpp-support-Lat.patch
0011-add-missing.patch
0012-add-layout-height-width-and-depth-functions.patch
0013-fix-some-names.patch
0014-fix-type-error.patch

I would guess that you don't want 0002, 0003, 0004, and 0007. The rest impact source, autoconfig builds, documentation, features, and/or build issues.

Based on research into this issue, freetype 2.13.3 changed some return
types that caused current (modern?) builds to fail as seen in the linked
ticket.

A patch was applied with frankheckenbach/ftgl#20 that appears to fix
this, but the repo owner has not indicated a plan to cut a new release
with this and other fixes since v2.4.0.

In addition to the patch files that were previously present, I have
applied all other patches from v2.4.0 to current HEAD. This involved
removing the existing patch files, but those changes have been fully
incorporated.

Builds with +doc and +universal variants worked fine and I have tested
a downstream port (gource) with this on an affected system.

macOS 15.2 24C101 arm64
Xcode 16.2 16C5032a

Closes: https://trac.macports.org/ticket/71434
@halostatue
Copy link
Contributor Author

I have minimized the patch set and added comments on how the patches were made.

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

Successfully merging this pull request may close these issues.

3 participants