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

qt5-qtwebkit: Flag conflicting ports on case insensitive filesystems #16890

Merged
merged 1 commit into from
Dec 25, 2022
Merged

qt5-qtwebkit: Flag conflicting ports on case insensitive filesystems #16890

merged 1 commit into from
Dec 25, 2022

Conversation

jhoyt4
Copy link
Contributor

@jhoyt4 jhoyt4 commented Dec 4, 2022

On some builds, the include search path picks up header files from macports
and occasionally the system before the local header files causing an error.
This modification places the qt5-qtwebkit header files first on the search path.

Closes: https://trac.macports.org/ticket/63877
Closes: https://trac.macports.org/ticket/62027
Closes: https://trac.macports.org/ticket/66177
Closes: https://trac.macports.org/ticket/63566

[skip ci]

Tested and verified to work with Test Application

Description

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

macOS 13.0.1 22A400 arm64
Xcode 14.1 14B47b

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?
  • checked your Portfile with port lint --nitpick?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
  • checked that the Portfile's most important variants haven't been broken?

@macportsbot
Copy link

Notifying maintainers:
@MarcusCalhoun-Lopez for port qt5.

@ostefano
Copy link
Contributor

ostefano commented Dec 23, 2022

This fixes the issue, thx!

Looking forward to have it merged :)

@chrstphrchvz
Copy link
Contributor

I probably cannot offer a constructive alternative to this PR anytime soon.

However I have not seen explanation for why this issue is specific to ARM64, and so do not see why the proposed change should be specific to one architecture (even if that is all it is commonly reported for); if the actual cause is something like filesystem case insensitivity, then it definitely should be applied to all architectures. And if it is not ARM64 specific, then older qtwebkit ports are likely affected, and the same fixes should be applied eventually for consistency.

Also, does the proposed approach of removing -I${prefix}/include mean that qtwebkit would no longer depend on certain other ports, i.e. installing qtwebkit would install unneeded dependencies?

@jhoyt4
Copy link
Contributor Author

jhoyt4 commented Dec 23, 2022

Honestly, I don't know why it is specific to ARM (all of my intel machines work fine...). I can easily remove the architecture if statement. I have successfully compiled and tested on intel with that change and did not see any adverse effects (only added the if statement out of the "if it ain't broke don't fix it" mentality).

From what I can tell in the build logs, the -I${prefix}/include gets added back in on the include search path - just after the qtwebkit internals with this modification. I haven't exhaustively checked the qtwebkit dependencies in a while, will do that when I get more time - but believe this to be safe (have been running on ARM with this workaround Nov 2022).

At any rate, this has been an issue for over a year. It would be nice if it eventually got addressed either by this proposed work around or something better.

@kencu
Copy link
Contributor

kencu commented Dec 23, 2022

it would sure be nice to know which external header(s) are causing the issue exactly, rather than this…

@jhoyt4
Copy link
Contributor Author

jhoyt4 commented Dec 23, 2022

Honestly, an exhaustive list would be near impossible to put together unless all ports were installed. Also, you get blocked one header at a time, so the ones I've found are:

"Archive.h"
"Event.h"
"unicode/UTF8.h"

@jhoyt4
Copy link
Contributor Author

jhoyt4 commented Dec 23, 2022

@chrstphrchvz

Here's the otool -L output for QtWebKit with correctly linked libs for icu leveldb webp libxml2 libxslt zlib sqlite3 libjpeg-turbo:

	/opt/local/libexec/qt5/lib/QtGui.framework/Versions/5/QtGui (compatibility version 5.15.0, current version 5.15.6)
	/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit (compatibility version 45.0.0, current version 2299.30.112)
	/System/Library/Frameworks/Metal.framework/Versions/A/Metal (compatibility version 1.0.0, current version 306.3.4)
	/opt/local/libexec/qt5/lib/QtNetwork.framework/Versions/5/QtNetwork (compatibility version 5.15.0, current version 5.15.6)
	/opt/local/libexec/qt5/lib/QtCore.framework/Versions/5/QtCore (compatibility version 5.15.0, current version 5.15.6)
	/System/Library/Frameworks/DiskArbitration.framework/Versions/A/DiskArbitration (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/IOKit.framework/Versions/A/IOKit (compatibility version 1.0.0, current version 275.0.0)
	/opt/local/lib/libxml2.2.dylib (compatibility version 13.0.0, current version 13.3.0)
	/opt/local/lib/libxslt.1.dylib (compatibility version 3.0.0, current version 3.37.0)
	/opt/local/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.13)
	/System/Library/Frameworks/IOSurface.framework/Versions/A/IOSurface (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1953.255.0)
	/opt/local/lib/libsqlite3.0.dylib (compatibility version 9.0.0, current version 9.6.0)
	/opt/local/lib/libleveldb.1.dylib (compatibility version 0.0.0, current version 0.0.0)
	/opt/local/lib/libjpeg.8.dylib (compatibility version 8.0.0, current version 8.2.2)
	/opt/local/lib/libpng16.16.dylib (compatibility version 56.0.0, current version 56.0.0)
	/opt/local/lib/libwebp.7.dylib (compatibility version 9.0.0, current version 9.5.0)
	/System/Library/Frameworks/Carbon.framework/Versions/A/Carbon (compatibility version 2.0.0, current version 169.0.0)
	/opt/local/lib/libicui18n.72.dylib (compatibility version 72.0.0, current version 72.1.0)
	/opt/local/lib/libicuuc.72.dylib (compatibility version 72.0.0, current version 72.1.0)
	/opt/local/lib/libicudata.72.dylib (compatibility version 72.0.0, current version 72.1.0)
	/opt/local/libexec/qt5/lib/QtSensors.framework/Versions/5/QtSensors (compatibility version 5.15.0, current version 5.15.6)
	/opt/local/libexec/qt5/lib/QtPositioning.framework/Versions/5/QtPositioning (compatibility version 5.15.0, current version 5.15.6)
	/opt/local/libexec/qt5/lib/QtQuick.framework/Versions/5/QtQuick (compatibility version 5.15.0, current version 5.15.6)
	/opt/local/libexec/qt5/lib/QtQmlModels.framework/Versions/5/QtQmlModels (compatibility version 5.15.0, current version 5.15.6)
	/opt/local/libexec/qt5/lib/QtWebChannel.framework/Versions/5/QtWebChannel (compatibility version 5.15.0, current version 5.15.6)
	/opt/local/libexec/qt5/lib/QtQml.framework/Versions/5/QtQml (compatibility version 5.15.0, current version 5.15.6)
	/opt/local/libexec/qt5/lib/QtMultimedia.framework/Versions/5/QtMultimedia (compatibility version 5.15.0, current version 5.15.6)
	/opt/local/libexec/qt5/lib/QtSql.framework/Versions/5/QtSql (compatibility version 5.15.0, current version 5.15.6)
	/System/Library/Frameworks/OpenGL.framework/Versions/A/OpenGL (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/AGL.framework/Versions/A/AGL (compatibility version 1.0.0, current version 1.0.0)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1300.36.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.0.0)
	/System/Library/Frameworks/CoreGraphics.framework/Versions/A/CoreGraphics (compatibility version 64.0.0, current version 1690.3.3)
	/System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices (compatibility version 1.0.0, current version 1228.0.0)
	/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation (compatibility version 300.0.0, current version 1953.255.0)
	/usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)`

I can't find a link to font-config in the installed files- but it might use one of the font-config installed executables.

@jhoyt4
Copy link
Contributor Author

jhoyt4 commented Dec 23, 2022

That last commit / force push was just to fix a typo in the comments as well as to add the latest qtwebkit ticket to the commit log.

Given that last tickets shows the issue occurring on intel, should I remove the if statement?

@kencu
Copy link
Contributor

kencu commented Dec 23, 2022

blocking all of macports' ports from being picked up is most likely going to cause a lot of trouble in the end.

Having the headers from who-knows-where linked up to macports libraries will also cause a lot of trouble in the end -- surprising, really, that it works at all now. Presumably ?? the icu headers from the system are being used, but the icu library from macports is being linked?? That just sounds like trouble waiting ...

@jhoyt4
Copy link
Contributor Author

jhoyt4 commented Dec 24, 2022

blocking all of macports' ports from being picked up is most likely going to cause a lot of trouble in the end.

I don't believe (but could be wrong) that the macports headers are being blocked. From looking at the build log on my machine, they get added back in, just later in the search path.

In regards to your now deleted question - "any guess then as to why this shows up now, 10 years in?" - no clue to the root cause, but more users will see this issue until pre-built packages are available on Ventura. I fear that this has been a hidden issue for some time, just masked by pre-built packages being available.

Also - this issue didn't just show up. It's been around for at least over a year. It would be nice to have a solution, even a bandaid solution, get applied until the root cause can fully be understood.

Troubleshooting the real issue is beyond my skillset. As always, I am happy to assist getting to the heart of the matter - but will need some help to get there.

@jhoyt4
Copy link
Contributor Author

jhoyt4 commented Dec 24, 2022

@kencu - To your question on if this works. It does. I have been successfully running with this work around for over a year without an issue.

Here is a screen shot from an application I built this week that uses qt5-qtwebkit. I picked cnn as the site so as to show "latest" news. Happy to grab a different screenshot from another site.

Screenshot 2022-12-24 at 8 55 17 AM

@jhoyt4
Copy link
Contributor Author

jhoyt4 commented Dec 24, 2022

Changing to the following seems to allow it to compile. Would this be an acceptable bandaid solution?:

                configure.cppflags-delete "-I${prefix}/include"
                configure.cppflags-append "-I${worksrcpath}/include:-I${prefix}/include"
                configure.cxxflags-delete "-I${prefix}/include"
                configure.cxxflags-append "-I${worksrcpath}/include:-I${prefix}/include"
                configure.cflags-delete "-I${prefix}/include"
                configure.cflags-append "-I${worksrcpath}/include:-I${prefix}/include"

@jhoyt4
Copy link
Contributor Author

jhoyt4 commented Dec 24, 2022

Before it's asked - just adding "configure.cppflags-append "-I${worksrcpath}/include" does not work. Unfortunately the delete and re-add is necessary.

@jhoyt4
Copy link
Contributor Author

jhoyt4 commented Dec 24, 2022

That last series of commits / force pushes added the updated code, removed the arm if statment and synced with the lastest macports master branch which had qt5 commits last night.

@kencu
Copy link
Contributor

kencu commented Dec 24, 2022

Yes, I do think that having the worksrcpath includes come first is a cleaner fix, thanks for thinking of that … maybe there is a clue in worksrcpath as to what headers are conflicting.

What we should hope to avoid most is mismatching headers and libraries. This has bitten us many times.

# Resolve this by making sure that the qt5-qtwebkit headers are first on the include path
# https://trac.macports.org/ticket/63877
configure.cppflags-delete "-I${prefix}/include"
configure.cppflags-append "-I${worksrcpath}/include:-I${prefix}/include"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I've ever seen a colon used in this position before... does that work?

clang++ -I/path/to/something:-I/opt/local/include test.cpp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope - that is a mistake. And unfortunately, the compile only worked due to the error. Substituting in a space for the colon does not build

@jhoyt4
Copy link
Contributor Author

jhoyt4 commented Dec 24, 2022

I just rolled back to a version with the "delete only lines".

The explicit error we keep tripping is this:

:info:build Modules/geolocation/Geoposition.h:30:10: warning: non-portable path to file '"event.h"'; specified path differs in case from file name on disk [-Wnonportable-include-path]
:info:build          "event.h"

@jhoyt4 jhoyt4 changed the title qt5-qtwebkit: Fix build on arm64 for Big Sur, Monterey, and Ventura qt5-qtwebkit: Fix build for Big Sur, Monterey, and Ventura Dec 24, 2022
@jhoyt4
Copy link
Contributor Author

jhoyt4 commented Dec 24, 2022

I just added another trac ticket to the commit log. So far, I've found 4 separate and related tickets for this issue.

@kencu
Copy link
Contributor

kencu commented Dec 25, 2022

I believe the conflict is coming from the "libevent" port then.

qt5-qtwebkit has a header called "Event.h" and libevent has a header "event.h"

% port contents libevent | grep event.h
  /opt/local/include/event.h

On the buildbot, Ryan uses a case-sensitive file system, so this error is not seen.

There seems to be another similar issue with libarchive, which provides "/${prefix}/include/archive.h".

@jhoyt4
Copy link
Contributor Author

jhoyt4 commented Dec 25, 2022

event.h: conflicts_build-append port:libevent
event.h (assumed...): conflicts_build-append port:ruby30
event.h: conflicts_build-append port:ruby31
archive.h: conflicts_build-append port:libarchive

@jhoyt4
Copy link
Contributor Author

jhoyt4 commented Dec 25, 2022

Is there a way to automatically deactivate / reactivate the conflicting ports or must this always be done manually?

@jhoyt4 jhoyt4 changed the title qt5-qtwebkit: Fix build for Big Sur, Monterey, and Ventura qt5-qtwebkit: Flag conflicting ports on case insensitive filesystems Dec 25, 2022
…r conflicts

  On case insensitive file systems, the include search path picks up header files from
  macports before the local header files causing an error while compiling.  This
  modification places flags the known conflicting ports with conflicts_build.

Closes: https://trac.macports.org/ticket/63877
Closes: https://trac.macports.org/ticket/62027
Closes: https://trac.macports.org/ticket/66177
Closes: https://trac.macports.org/ticket/63566

[skip ci]

fun
@kencu
Copy link
Contributor

kencu commented Dec 25, 2022

it can be done automatically, but the admins really frown on it, as it changes installed ports without the user’s approval or knowledge…

I feel somewhat better understanding the issue more completely. We have a couple of options to fix this as you noted, but no ideal option. The best option might be to do as you said, prioritize WebKit’s headers. Sometimes the headers in ${prefix} can be prepended to the system header list… I have seen that done successfully.

I just built this successfully without having the libevent or libarchive ports installed. As you said, it’s possible some other may turn up.

Let’s get this in, and then if I (or you) can come up with something more elegant, we can do that later.

Thanks for your patience as I fussed about the real issue…

@kencu kencu merged commit 8b52834 into macports:master Dec 25, 2022
@jhoyt4
Copy link
Contributor Author

jhoyt4 commented Dec 25, 2022

@kencu - Thank you for hanging in with me on this one. I'm very much learning as I go, and your assistance and guidance has helped a ton.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer: open Affects an openmaintainer port type: bugfix
Development

Successfully merging this pull request may close these issues.

6 participants