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

qtbase: Fix paths returned by qmake -query #57097

Merged
merged 1 commit into from Jul 15, 2019

Conversation

FlorianFranzen
Copy link
Contributor

@FlorianFranzen FlorianFranzen commented Mar 8, 2019

Motivation for this change

Some project rely on the output of qmake's property mode (qmake -query) to find their dependencies, which currently returns the incorrect paths for any Qt files not installed into the main output path of qtbase, because they are installed in into a secondary output (namely Headers and Plugins), do not follow Qt path standard (Docs and Plugins) or are part of a different module (e.g qtmultimedia, etc.).

qmake computes these paths using the QLibrarySettings class compiled with QT_BUILD_QMAKE defined. The class uses QT_CONFIGURE_PREFIX_PATH as the default prefix, and appends certain default suffixes as definded in qtConfEntries inside the QLibrarySettings implementation. These can however be overwritten using a qt.conf INI file, which is what this patch does to override the path of Headers, Plugins and Documentation. The Prefix also needs to be supplied as it would default to the current working directory else. This fix of course only works for qtbase, as any other moduel (e.g. qtmultimedia) would need their own qt.conf and symlink to qmake, which would introduce other PATH related issues itself and therefore is not in the scope of this patch

Fixes #56357.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@FlorianFranzen
Copy link
Contributor Author

Rebased patch against current staging, because of large number of rebuilds.

@infinisil
Copy link
Member

I thought this would eliminate the need for patches like this one, but I tried it, and it doesn't seem to fix it. Or is this something else?

@FlorianFranzen
Copy link
Contributor Author

@infinisil: As Qt by default only checks the application directory for a qt.conf file, the file generated with this patch most likely has no effect. Also the QLibraryInfo class act slightly differently if compiled for qmake, which might also have effect.

More information about the mechanism can be found in the qt.conf chapter of the official documentation.

@FlorianFranzen
Copy link
Contributor Author

@qknight and @ttuegel, any feedback would be welcome.

@FlorianFranzen
Copy link
Contributor Author

@infinisil @qknight @ttuegel Please advice on what to do with these changes or how to work around the broken qmake -query support in Nix and NixOS.

@@ -376,6 +376,18 @@ stdenv.mkDerivation {
moveToOutput bin "$dev"
''

# Fix paths returned by qmake -query
+ ''
cat > $dev/bin/qt.conf <<EOF
Copy link
Member

Choose a reason for hiding this comment

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

What will happen if the dev output is installed in a user environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I know all the effects of a derivation being installed in a user environment as I am by far not a Nix expert, but as long as that only entails the bin subfolder being added to the PATH, the added file should have no side effects, at least according official documentation on qt.conf:

QLibraryInfo will load qt.conf from one of the following locations:

- :/qt/etc/qt.conf using the resource system
- on macOS, in the Resource directory inside the application bundle, for example assistant.app/Contents/Resources/qt.conf
- in the directory containing the application executable, i.e. QCoreApplication::applicationDirPath() + QDir::separator() + "qt.conf"

The qt.conf file should only really affect the binaries inside the derivations bin folder

Copy link
Member

Choose a reason for hiding this comment

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

The qt.conf file should only really affect the binaries inside the derivations bin folder

Installing it into a user environment will cause it to be symlinked into another directory with (symlinks to) other programs. I'm concerned that this will break other Qt programs if QCoreApplication::applicationDirPath is based on argv[0]. I will check...

Copy link
Member

Choose a reason for hiding this comment

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

It looks like QCoreApplication::applicationDirPath gets the program filename from /proc, which is fine, but it does fall back to argv[0]. So, I think this change should be fine.

@matthewbauer
Copy link
Member

ping @ttuegel

@aanderse
Copy link
Member

Anything outstanding on this PR? (triage)

@ttuegel ttuegel merged commit e92a2f2 into NixOS:staging Jul 15, 2019
@samueldr
Copy link
Member

This PR broke qt-full-5*

Hydra evals:

@vcunat
Copy link
Member

vcunat commented Jul 26, 2019

I can confirm that reverting this merge atop current master fixes the build of qt5Full (tried just locally).

@ttuegel
Copy link
Member

ttuegel commented Jul 26, 2019

@vcunat @samueldr Thanks for tracking this down! I think we should revert while we figure it out. Any objections?

@samueldr
Copy link
Member

@ttuegel sounds okay, I'm leaving the decision up to you.

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

8 participants