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

Fix qtPrepareTool function #118904

Merged
merged 4 commits into from May 9, 2021

Conversation

OPNA2608
Copy link
Contributor

@OPNA2608 OPNA2608 commented Apr 9, 2021

Motivation for this change

Fixes #76080. Extracted from #108366.

What this fixes / Why this is needed

Loading qtbase's qt_module qmake feature in a project with git_build set (either manually set or when .git is present) will use the qtPrepareTool function to find Qt's syncqt.pl tool. This function, in its upstream implementation, uses the $$[QT_HOST_BINS] variable to reference the <qt-installation>/bin directory with all of Qt's modules installed, appends the requested tool name to it ($${2}) and checks for some variations of this path (.pl, .exe, .app/Contents/MacOS/$${2}), falling back to just the initial concatenation.

The current patch only replaces this concatenation ($$[QT_HOST_BINS]/$$2) with a command -v $${2} invocation (because we don't keep all tool binaries/scripts in the same directory), leaving the following extension-handling code unchanged. This functions fine for binary tools without any further file extension or path variation (like moc), but fails on syncqt.pl (empty response from command -v syncqt + .pl) and falls back to an empty string. This is an unexpected result for the rest of the feature code, which appends arguments to the result and tries to invoke the tool. Hence you may get:

Project MESSAGE: -module QtWebEngineCore -version 6.0.0 -outdir /home/michael/src/tmp/qtwebengine -builddir /home/michael/src/tmp/qtwebengine /home/michael/src/tmp/qtwebengine
sh: -module: not found
Project ERROR: Failed to run: -module QtWebEngineCore -version 6.0.0 -outdir /home/michael/src/tmp/qtwebengine -builddir /home/michael/src/tmp/qtwebengine /home/michael/src/tmp/qtwebengine

instead of:

perl -w /nix/store/22ll16l5gmx5dgq98a1i5v5p3ss9sjzb-qtbase-5.15.2-dev/bin/syncqt.pl -module QtWebEngineCore -version 6.0.0 -outdir /home/michael/src/tmp/qtwebengine -builddir /home/michael/src/tmp/qtwebengine /home/michael/src/tmp/qtwebengine

Or sometimes you may get no indication of any missing tool invocation at all, followed by missing header errors because the tool that would've generated them wasn't executed.

This PR fixes the patch.

  1. We check for a binary, extension-less tool first: command -v $${2}. If a match for the tool name is found, we exit the finding code with its full path as the command.
  2. Next we check for a Perl tool via command -v $${2}.pl. If a match is found then perl -w ${/path/to/tool.pl} is used as the command.
  3. Lastly we check some platform-specific extensions for binary tools (.exe on Windows, .app on Darwin). If these aren't found either then we give up and continue with an empty command name again.

(I'm unsure if it would make sense to insert a known-bad command if no match was found. Sometimes qtPrepareTool seems to get used to check if an optional tool like qdoc exists, which always fails and doesn't seem to cause any problems?)

To test, try building qtsvg (looks for moc binary tool, sanity check) and qtwebengine (looks for syncqt Perl tool, I've removed the workaround that was added in #116724)

With this fix:

make[2]: Entering directory '/build/source/src/core'
( test -e Makefile.core_headers || /nix/store/22ll16l5gmx5dgq98a1i5v5p3ss9sjzb-qtbase-5.15.2-dev/bin/qmake -o Makefile.core_headers /build/source/src/core/core_headers.pro PREFIX=/nix/store/nywpzvcln3x5bc40sjrabrw8065kmnqq-qtwebengine-5.15.3-a059e74 NIX_OUTPUT_OUT=/nix/store/nywpzvcln3x5bc40sjrabrw8065kmnqq-qtwebengine-5.15.3-a059e74 NIX_OUTPUT_DEV=/nix/store/hxsbskrarzwhm2p2vsipq97z8kwzbygl-qtwebengine-5.15.3-a059e74-dev NIX_OUTPUT_BIN=/nix/store/hy73pygc6p5j3k7kfsmdrf71n38jnibz-qtwebengine-5.15.3-a059e74-bin NIX_OUTPUT_DOC=/nix/store/hxsbskrarzwhm2p2vsipq97z8kwzbygl-qtwebengine-5.15.3-a059e74-dev/share/doc/qt-5.15.2 NIX_OUTPUT_QML=/nix/store/hy73pygc6p5j3k7kfsmdrf71n38jnibz-qtwebengine-5.15.3-a059e74-bin/lib/qt-5.15.2/qml NIX_OUTPUT_PLUGIN=/nix/store/hy73pygc6p5j3k7kfsmdrf71n38jnibz-qtwebengine-5.15.3-a059e74-bin/lib/qt-5.15.2/plugins CONFIG+=release ) && make -f Makefile.core_headers 
Project MESSAGE: perl -w /nix/store/22ll16l5gmx5dgq98a1i5v5p3ss9sjzb-qtbase-5.15.2-dev/bin/syncqt.pl -module QtWebEngineCore -version 5.15.3 -outdir /build/source -builddir /build/source /build/source
<srcbase> = /build/source 
<bldbase> = /build/source 
<outbase> = /build/source 
QtWebEngineCore: created fwd-include header(s) for <srcbase>/src/core/api/ { qtwebenginecoreglobal.h (1), qtwebenginecoreglobal_p.h (1), qwebenginecallback.h (2), qwebenginecallback_p.h (1), qwebengineclientcertificatestore.h (2), qwebenginecookiestore.h (2), qwebenginecookiestore_p.h (1), qwebenginefindtextresult.h (2), qwebenginehttprequest.h (2), qwebenginemessagepumpscheduler_p.h (1), qwebenginenotification.h (2), qwebenginequotarequest.h (2), qwebengineregisterprotocolhandlerrequest.h (2), qwebengineurlrequestinfo.h (2), qwebengineurlrequestinfo_p.h (1), qwebengineurlrequestinterceptor.h (2), qwebengineurlrequestjob.h (2), qwebengineurlscheme.h (2), qwebengineurlschemehandler.h (2) }

Without the fix and without the workaround script (copied from the linked PR, haven't had the time to fully compile qtwebengine yet):

make[2]: Entering directory '/build/qtwebengine-everywhere-src-5.15.2/src/core'
( test -e Makefile.core_headers || /nix/store/rbwz406jsdnjg7lglyvwdpwhni2fgjys-qtbase-5.15.2-dev/bin/qmake -o Makefile.core_headers /build/qtwebengine-everywhere-src-5.15.2/src/core/core_headers.pro PREFIX=/nix/store/mpk1sfy77yqwf5rffc18wwkzhs4vdx1d-qtwebengine-5.15.2 NIX_OUTPUT_OUT=/nix/store/mpk1sfy77yqwf5rffc18wwkzhs4vdx1d-qtwebengine-5.15.2 NIX_OUTPUT_DEV=/nix/store/q0vi1drd7mfvpkq44dv04b084sb9i41f-qtwebengine-5.15.2-dev NIX_OUTPUT_BIN=/nix/store/v8m26xlb5lc4nbm7irxdwwiiyp32r142-qtwebengine-5.15.2-bin NIX_OUTPUT_DOC=/nix/store/q0vi1drd7mfvpkq44dv04b084sb9i41f-qtwebengine-5.15.2-dev/share/doc/qt-5.15.2 NIX_OUTPUT_QML=/nix/store/v8m26xlb5lc4nbm7irxdwwiiyp32r142-qtwebengine-5.15.2-bin/lib/qt-5.15.2/qml NIX_OUTPUT_PLUGIN=/nix/store/v8m26xlb5lc4nbm7irxdwwiiyp32r142-qtwebengine-5.15.2-bin/lib/qt-5.15.2/plugins CONFIG+=release ) && make -f Makefile.core_headers 
make[3]: Entering directory '/build/qtwebengine-everywhere-src-5.15.2/src/core'
make[3]: Nothing to be done for 'first'.
make[3]: Leaving directory '/build/qtwebengine-everywhere-src-5.15.2/src/core'
…
/build/qtwebengine-a059e74/src/core/api/qtwebenginecoreglobal_p.h:54:10: fatal error: QtWebEngineCore/qtwebenginecoreglobal.h: No such file or directory
   54 | #include <QtWebEngineCore/qtwebenginecoreglobal.h>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
    (Only qtbase, some qtModules and afew minutes of qtwebengine)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@OPNA2608
Copy link
Contributor Author

(Unsure about the Darwin one, I don't think application bundles are usually found on the PATH? Maybe the Darwin conditional one shouldn't be part of that elseif and should instead be looked up earlier, with the extensionless fallback still functional)

Confirmed that this fails to find moc on Darwin. Will restructure the patches to do $${2} -> $${2}.pl -> platform-specific stuff.

@OPNA2608 OPNA2608 marked this pull request as draft April 12, 2021 17:11
@OPNA2608 OPNA2608 force-pushed the fix/qtbase-mkspecs-patch/21.05 branch from 173f0cb to 5522611 Compare April 13, 2021 09:40
@OPNA2608
Copy link
Contributor Author

OPNA2608 commented Apr 13, 2021

I think I fixed it, will rerun some test builds on my darwin system later today.

EDIT: Been awhile but IIRC qtsvg and qtwebengine build fine on Darwin now.

@OPNA2608 OPNA2608 marked this pull request as ready for review April 13, 2021 09:42
@OPNA2608 OPNA2608 force-pushed the fix/qtbase-mkspecs-patch/21.05 branch from 5522611 to 108fb20 Compare April 13, 2021 09:46
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/508

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

Ran most of a review, didn't see many failures specific to qt.

Likely missed somethign with all the staging noise.

@jonringer jonringer merged commit 77b148d into NixOS:staging May 9, 2021
@OPNA2608 OPNA2608 deleted the fix/qtbase-mkspecs-patch/21.05 branch September 27, 2022 17:51
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

4 participants