qt6: don't treat absolute paths without known suffix as library #367206
Conversation
pitkling
force-pushed
the
sioyek-fix-darwin-build
branch
from
3dec620 to
0f7cfda
Compare
|
Please retarget this to staging. Also, is upstream aware of this at all? |
pitkling
force-pushed
the
sioyek-fix-darwin-build
branch
from
0f7cfda to
2364dab
Compare
github-actions
bot
added
6.topic: python
6.topic: nixos
Ah sorry, I wasn't aware of the mass-rebuild rule for contributions. I retargeted the PR, hope it's correct now. Note that I didn't finish building sioyek with this retargeted PR locally yet to test the fix, it's taking a while…
Not yet, since I wanted to confirm first whether nixpkgs's current darwin behavior (as it was in Qt 6.7.2 and again after this PR) is intended/correct. That is, whether something like the following excerpt from In particular, is the first entry the intended linker flag to be generated or should it be also a |
ofborg
bot
added
the
2.status: merge conflict
Looking at the linker flags for sioyek (without this PR): That already includes the framework flag directly after the lib flag: The |
ofborg
bot
removed
the
2.status: merge conflict
|
Just FYI @K900: I just posted a comment in the original upstream issue that was targeted by the mentioned upstream commit to ask whether prepending the |
pitkling
force-pushed
the
sioyek-fix-darwin-build
branch
from
2364dab to
55b3d70
Compare
|
@K900: I submitted the change from this PR as an upstream fix which has been reviewed and accepted. It is currently in the staging/integration cycle. I assume it's gonna take a while until an updated Qt version with the patch hits nixpkgs. Should we include the patch from this PR until it does? I converted this from draft PR to a normal PR and rebased onto current staging. |
|
Thanks! Let's backport this for the time being. |
|
Great, thanks for the quick merge! Quick question: For this to eventually hit 24.11 instead of only unstable, do I have to create a back port PR? |
|
Just adding the label should trigger the automation, assuming it applies cleanly. |
|
Successfully created backport PR for |
In the update from 6.7.2 to 6.7.3, commit `ea0f00d5d` changed how linker flags like `ws2_32` are treated when generating pri/prl files. Specifically, before the commit a flag like `ws2_32` was left untouched. The above commit changed it such that such flags are converted to `-lws2_32` (seemingly in order to better support FFmpeg, according to the commit message). However, this change also affects absolute paths if the file has no extension. That is, after the above mentioned commit, an absolute path linker flag to, say, a dylib on macOS without a suffix will result in prepending the `-l` flag. This will result in errors during link time. An example where this caused problems can be found in the nixpkgs PR draft #367206 (NixOS/nixpkgs#367206). This adds a small check to ensure that `-l` is not prepended if the linker flag is an absolute path without a suffix. Change-Id: I2c7ce3aac6624e1a27c59af233e3da2c1ae7ba60 Reviewed-by: Alexey Edelev <[email protected]>
Fixes issue #366069 (broken sioyek darwin build) by adapting an upstream change when Qt was updated from 6.7.2 to 6.7.3 in PR #344928. Might fix other similar issues (e.g., for packages depending on
qt6.qtspeech).I submitted the same fix upstream which has been accepted and is currently in the staging/integration phase.
Details
(Note: Take the following with a grain of salt, as I've not much experience with cmake/qmake/Qt.)
This upstream commit changed how Qt treats linker flags without a suffix (like
.so) when generating.pri/.prlfiles. Until 6.7.2, a linker flag likews2_32was left untouched. The above commit changed it such that such flags are converted to-lws2_32(seemingly in order to better support FFmpeg, according to the commit message).Now, nixpkgs on darwin links some libraries, like
QtMultimediavia framwork paths like/nix/store/wmm6s68wk9ygg84ibzdf95yp22lcg4ay-qtmultimedia-6.7.3/lib/QtMultimedia.framework/Versions/A/QtMultimedia. As long as these are part of the current build dir or of Qt's$prefix/libdir, they are recognized as frameworks and handled accordingly. This works, e.g., for something like/nix/store/rdjd1nqlr1ncr4616dcyqdf6ymqy82xc-qtbase-6.7.3/lib/QtGui.framework/Versions/A/QtGuisince it is part ofqt6.qtbase.However, QtMultimedia is not part of
qt6.qtbase, so/nix/store/wmm6s68wk9ygg84ibzdf95yp22lcg4ay-qtmultimedia-6.7.3/lib/QtMultimedia.framework/Versions/A/QtMultimediais not recognized as a framework path and, instead, treated like a non-Qt library. Now, before the above mentioned upstream commit, the linker flag was handed over toclang++unchanged. After the upstream commit, it is transformed into-l/nix/store/wmm6s68wk9ygg84ibzdf95yp22lcg4ay-qtmultimedia-6.7.3/lib/QtMultimedia.framework/Versions/A/QtMultimedia, which causes a linker error since it's not a valid linker flag (the file is a dylib).This commit adds a patch that slightly adapts the upstream commit. Namely, it only prepends flags without a suffix (like
ws2_32) with an-lif they are not given as an absolute path.Additional Remarks
WIth this fix, the generated
prlfiles look again exactly as before the update from Qt 6.7.2 to 6.7.3 and linking of packages like sioyek works again. However, I'm not sure whether this is actually the correct way to fix the issue.It seems to me that instead of handing
/nix/store/wmm6s68wk9ygg84ibzdf95yp22lcg4ay-qtmultimedia-6.7.3/lib/QtMultimedia.framework/Versions/A/QtMultimediathrough toclang++, it should be converted into-framework QtMultimedia(with a suitable-F …flag). In fact, somehow these flags do end up in the actual linker command, not sure how though (maybe due to the recent Apple SDK improvements?).Also, I am surprised thatI misinterpreted the output path,clang++does not choke on getting simply a directory as an argument./nix/store/wmm6s68wk9ygg84ibzdf95yp22lcg4ay-qtmultimedia-6.7.3/lib/QtMultimedia.framework/Versions/A/QtMultimediais actually a dylib file. So the absolute path makes sense for linking.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Pinging Maintainers etc.
Add a 👍 reaction to pull requests you find important.