-
-
Notifications
You must be signed in to change notification settings - Fork 15k
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
Loads qt plugin paths as registered... #44047
Loads qt plugin paths as registered... #44047
Conversation
233f123
to
b0d74d8
Compare
local inputs | ||
|
||
# FIXME : this causes output path cycles... | ||
# I am unsure if it is even needed, though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and there's this...
I'm not sure I need to do this, but maybe I need, otherwise some parts may be missing.
The situation I'm thinking about is just like how qtbase
has dev
and bin
. When you buildInputs = [ qt5.qtbase ]
you get qt5.qtbase.dev
. This is fine, except the plugins are in qt5.qtbase.bin
, which won't get figured out. This is why I'm forcibly adding it to nix-support/qt-plugin-paths
at build time.
I'll have to first wrap my mind around what the error is, then figure out something.
cycle detected in the references of '/nix/store/dkaprffmpi2aql8c0a2kxsid9pfvf5zj-qtsvg-5.11.1' from '/nix/store/v91n5pqk908brijpcd23wfrp4pkrg5g0-qtsvg-5.11.1-bin'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The situation I'm thinking about is just like how
qtbase
hasdev
andbin
.
This is the situation for almost all libraries. I noticed that you hardcoded the dependency on qtbase
below; wouldn't we need to do this for all Qt libraries? (Missing the plugins from qtbase
is the usually the only problem that is fatal, but missing the plugins from other libraries still leads to strange behavior and missing features.)
Hmm... I don't think it should have to be so much extra stuff. You should be able to reuse qtbase.patch and do something like:
and a definition for QTBASEDIR here: On closure size, I actually don't think this will have a huge effect. IIRC qtbase is leaking into it anyway so I wouldn't worry about it much:
|
Shortened quote with what I understand of it. That would work fine for
This does not only provide the platforms (like libqxcb), but also provides many parts for Qt applications to compose themselves with
And as for "You should be able to reuse qtbase.patch", this is already noted as things to do (Merge with other qtbase patches? If not, at least rename the patch.) As for right now, keeping the patch separate makes it easier for me to adjust as needed, and makes it easier to review (I hope). |
Had a quick ask-around on IRC today and @acowley replied with stuff I could try.
|
Progress! Looks like that with the patch applied, using Though, this may be solvable. Playing around with the env, I was able to reduce it to two environment variables (both need to be unset).
This means that for Qt things, tests inside and outside Plasma will need to be done. (Good to know!) For the record, I have built a 5.11.0 equivalent without the patch to compare Running without touching the environment results in:
As it tried using the As expected, unsetting both previous values doesn't change a thing. This means that at the current stage: there is improvement, and no regression as far as I tested. Exploring the variablesThey both are only used in This probably means that it is rooted into one of the 11 files using this function. A chunk of those are platform definitions, which can be discarded. Left are three files, one using it for indicator support under unity (not an issue here), another using it for font configuration under unity and gnome (not an issue here). Looks like It is Qt's integration for KDE theming that causes issues. unsetting Further investigation is needed, but this looks like it could be solvable if |
Sorry I misunderstood what Qt plugins were for. It looks like you're doing great work here! I would definitely recommend getting this reviewed from @ttuegel who should be able to provide feedback on some of the internals of Qt stuff. |
Maybe you know, whether or not github's CODEOWNER notifications are noisy enough that @ttuegel would have seen it without mentioning? Thanks for the words, it's not initially obvious that so much is built as plugins in Qt. |
Success!
@ttuegel any particular reason for using the major.minor only? Qt seems to dislike mis-matched libraries, at least going from higher to lower patch releases. For the record, here's the VM I'm using for testing, with glorious hardcoded paths in the build.sh script. |
Is there currently a workaround to use the affected programs ? |
Qt claims that there is forward and backward binary compatibility along I have not had time to review this in depth, but the general principle seems sound to me. Thanks for tackling this! |
I still need to test an assumption before speaking (and I hate to comment before testing it), but I think the hook might not solve the most damaging bug (#47552) where installing through nix-env can break the whole plasma session. (This is 100% an assumption and not verified, I have not even checked the wrapper's implementation and may be 100% wrong and kinda hope to be wrong here.) I also need to re-check whether this PR (#44047) fixes it, I think I verified and it did, but I need to re-check. The main issue here is that I need to go back to a 5.11 Qt world since it needs to happen between two minor releases (and maybe other factors?). |
That bug is caused by having plugins installed into profile paths, which get picked up by Plasma packages. I did not see anything in this pull request to address that. Could you explain how this works to solve that problem?
Actually, this should already be dealt with by having a different
My experience has been that some packages always fall outside our attempts to automate. That is also true of the hook I wrote for #54525. The burden of debugging corner cases always falls on individual maintainers; in that case, the wrapper is easier to debug than a patched Qt. I want to automate packaging as much as possible, but not at the expense of giving packagers better tools when things go wrong. |
This adds the list of Qt plugin paths into builds using `qtbase`. The plugins list will be added to the Qt plugin path through the additional patch. This ensures that no special environment is required to get the desired plugin path for Qt software. This should fix all issues where Qt is unable to load its platform plugin. This may fix the issue where Qt tries to load a mismatched version of a plugin.
a0b1624
to
475c7c5
Compare
After some compilation time, yes, #47552 is handled by this PR.
I think you're right, re-reading the implementation, and thinking about how it works, nothing specifically addresses the issue, but the way the dependencies are resolved helps. Instead of going through the PATH elements, which may end up providing unmatched plugins, it will instead first look at what it was built with, providing working plugins. The implementation basically bundles a QT_PLUGIN_PATH internally inside the built derivation, in nix-support, which the patched Qt knows to look for first. It might be possible to get unmatched plugins, I'm not a Qt expert so bear with me, if e.g. a specialized "image" plugin was installed in the profile, while only the basic image plugins were provided in the built derivation.
I might have misspoke and meant patch level. The easiest way to trigger the issue is with patch levels, assuming |
That was my thought, too. #54525 addresses this the same way: paths registered at build-time are searched first, then we go through
So, Qt-upstream claims that the |
_Qt_sortless_uniq() { | ||
# `uniq`, but keeps initial order. | ||
# This is to remove risks of combinatorial explosion of plugin paths. | ||
cat -n | sort -uk2 | sort -nk1 | cut -f2- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to exist at https://stackoverflow.com/a/20639730 - Stackoverflow links are sometimes worthwhile comments
+ // Start at the binary; this allows us to *always* start by stripping the last part. | ||
+ QStringList components = applicationFilePath().split(QDir::separator()); | ||
+ | ||
+ // We don't care about /nix/store/nix-support, only /nix/store/*/nix-support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am uncomfortable with searching upward from the executable in this way. Going up one level should be fine (from /nix/store/.../bin/foo
to /nix/store/.../nix-support/
). The part about components.length() > 3
smells fishy to me; what happens if Nix is built with a different store path, such as a user-mode installation?
+ // This is why we're checking for more than 3 parts. It will bail out once /nix/xtore/*/nix-support/qt-plugin-paths has been tested. | ||
+ while (components.length() > 3) { | ||
+ components.removeLast(); | ||
+ const QString support_plugin_paths = QDir::cleanPath(QDir::separator() + components.join(QDir::separator()) + QStringLiteral("/nix-support/qt-plugin-paths")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QtDeclarative-based applications also need QML2_IMPORT_PATH
. Some Qt applications and all KDE applications need XDG_DATA_DIRS
and XDG_CONFIG_DIRS
. Could you extend this to cover those cases?
local inputs | ||
|
||
# FIXME : this causes output path cycles... | ||
# I am unsure if it is even needed, though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The situation I'm thinking about is just like how
qtbase
hasdev
andbin
.
This is the situation for almost all libraries. I noticed that you hardcoded the dependency on qtbase
below; wouldn't we need to do this for all Qt libraries? (Missing the plugins from qtbase
is the usually the only problem that is fatal, but missing the plugins from other libraries still leads to strange behavior and missing features.)
I updated nix package manager and all my installed apps. I am getting the same error for kolourpaint (kde app). :/ |
Closing since #54525 handles this issue. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/announcing-nomia-a-general-resource-manager-inspired-by-nix/12591/26 |
Motivation for this change
Fixes Qt plugin loading
This error message:
This is a sturdier(?) fix to #24256. The current fixes doesn't seem to work properly in some situation.
What is going on currently is that for any Qt plugin, they will be loaded according to the environment, and according to some hard-coded paths relative to components of PATH. This will not work properly when the environment is cleared or manipulated. (With non-NixOS platforms, using nix-shell to start a Qt application when none have been installed using nix may cause such failure.)
Furthermore, there is an issue. See how I said "components of PATH", well, the platform plugins are taken either from QT_PLUGIN_PATH (that's an upstream variable) or from a path relative to the PATH components.
The current qt5.qtbase.bin does not create
$bin/bin
, which is why (AFAICT) it is not added to the PATH. (In a previous revision, I tested adding abin
folder and it seemed to help.)What this change does is it collects all plugin paths from qtbase up to the built package, using a setup hook. The generated file is then found by the application and then used to seed the plugin paths.
It prefixes the plugin paths with those collected paths; this means that previous QT_PLUGIN_PATH and PATH based shenanigans should still work; it will prefer compiled-in paths. The behaviour is opt-out via
dontAddPluginPaths
though I'm not sure if there are any reasons to opt out.Possibly fixes mismatched Qt versions
I hope this fixes #30551, but I have a hard time reproducing the issue. The issue, AFAICT, is when software is compiled with Qt 5.X.Y and ran with 5.X.Z, where both X are the same, and Y and Z differ.
I would like a step-by-step repro of this issue, as I was unable to reproduce it faithfully. I got it one time when trying to force it, but I must have goofed up when reproducing it, as I was unable to reproduce it.
Request for comments
First of all, let's talk about the implementation. If the implementation seems right, THEN we'll talk about the colour of the bike shed.
That said, I have a couple questions.
The approach
Is it a good approach to save those plugin paths that way?
I personally feel it's right, otherwise those compiled application will search wherever to find a match for a probably Qt library. This means that installing in your environment from a different channel may cause either system apps or user environment apps to stop working. This should fix this issue.
Then, are the details right enough? Should
nix-support
be used for this, or are there more appropriate folders?First time setup hook
Is this done correctly? I think that unless someone removes genericBuild from their builders, this should always run when
qtbase
or any packages depending onqtbase
are in the mix, right?Issue with cyclic dependencies
There is a
FIXME
comment in the setup-hooks. See the (github) comment I left in the setup hooks file.C++/Qt code quality
I always hate hacking on C++, I know enough of it to be dangerous, I'd even say I know enough of it to know what I did is probably not bad, but I'm not sure if it's good.
First, the C++ code itself I think is ok. RAII makes it so I don't have to release anything I added?
Then, Qt code, are there any tricks, shortcuts I could have used?
I think climbing up from
::applicationFilePath
is sane, then I'm looking up until I reach the nix store. This means that applications deeply nested (e.g. libexec) will also be able to read the paths.Testing this
Any known recalcitrant Qt packages to test? Right now I haven't made in-depth tests, only superficially verified it works. I will test a bunch more later, unless there are known trick to test this.
As for what I tested, here is an example using
quaternion
.I also verified with
strace
that it reads the paths insidenix-support
.Using
dontAddPluginPaths
I also verified how apps will work if they don't find thenix-support
path; they simply go back to working the way they worked before.What's left to do?
Commit message:
Things done
Additional notes: