-
-
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
masterpdfeditor: 5.1.12 -> 5.1.60 #46579
Conversation
|
||
buildInputs = [ nss qtbase qtsvg sane-backends stdenv.cc.cc ]; | ||
|
||
dontStrip = true; | ||
|
||
postInstall = '' | ||
wrapProgram $out/bin/masterpdfeditor5 --prefix QT_PLUGIN_PATH : ${qtbase}/lib/qt-5.${lib.versions.minor qtbase.version}/plugins |
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.
What problem does this wrapper solve? The package builds and runs fine without it on my machine.
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 encountered the same problem as: #24256 and I was able to fix it with this wrapper. What's weird is that we don't have the same error. I didn't need this wrapper with 5.1.12, only with 5.1.36.
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.
That's a known error when you try to run Qt apps on a machine which has another Qt version installed. I'm not sure if we should add a wrapper to fix this because this may cause other problems. What does our Qt specialist @ttuegel recommend?
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.
Wrapping the program like this will surely break it for someone. Failing to wrap the program will also surely break it for someone. There is practically no way to support Qt applications in Nixpkgs in every configuration; something is going to be broken. The only important thing is that it works for you, the maintainer. If you're going to wrap, the QT_PLUGIN_PATH
entry should be ${lib.getBin qtbase}/${qtbase.qtPluginPrefix}
instead of what is above.
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.
ping @CMCDragonkai @flokli
I thought we no longer needed makeWrapper due to some trick. |
@CMCDragonkai The "trick" must be this one: #24256 (comment), which unfortunately isn't enough - see #44047. I poked through other changes referring to that issue, and am fine with adding the wrapper, with a comment above like in https://github.com/NixOS/nixpkgs/pull/43390/files#diff-c2877cd32bac930d3156eb07aa8821fdR62 @apeyroux can you update your PR with the comment and proper wrapping path? |
@apeyroux can you prefix "masterpdfeditor: " on the first commit msg of this PR? Apart from that, ready to be merged :-) |
615e971
to
27298d3
Compare
Thanks! |
Could someone please backport this to the release channel(s), the 5.1.12 source 404s. |
Thanks! Sadly, it seems like now they released 5.2.20 and 5.1.60 is 404ing. Does anyone have any contact with upstream? Their behavior is really annoying. |
up to 5.2.20 -> #52871 |
Motivation for this change
masterpdfeditor: 5.1.12 -> 5.1.36
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)