-
-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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: build wkhtmltopdf with patched Qt4 #96379
Fix: build wkhtmltopdf with patched Qt4 #96379
Conversation
I'm OK with keeping it working with Qt4 if possible. wkQt on master probably does not include webkit due to #92261 and missing dependencies. You can either add them to the overridden buildInputs or replace |
I have added a commit that fixes the build on master. It was failing due to gcc9 update. gstreamer turned out to be unnecessary. |
Ok, sorry, I was on vacation. Nevertheless: I've removed the trailing whitespace accident and hope that checks will be fine now. Anything else needed before we can merge this? |
Pinging @jb55 to get his blessing on this. I just reviewed this again and this is the same patch we've been running at FCIO for a while now. :) |
don't need my blessing, I haven't been homesteading this for a long time now. happy to pass maintainership |
Great, so now that this has been around for a while and I wanted to push it on for merging, we got reports that this build causes incorrect renderings. I have a version around that has updated build settings that work better. Does anyone have an idea for how to validate rendering quality through tests? In particular the current build seems to break with some CSS issues and using the upstream build options from the official build automation works. I'll update this PR later today. |
I marked this as stale due to inactivity. → More info |
@ctheune Thanks for bringing up the issue! |
Hey @grassit I'm not sure what you're asking. Your question seems not really related to this change here and is quite open-ended. If you're asking about general build issues with wkhtml then I'd suggest contacting their mailing list. Unfortunately I'm too busy to write down an overview of the build process. The download page has a few bits about it, though: https://wkhtmltopdf.org/downloads.html |
Have you / Do you need to update the PR with your changes?
I'm not sure but if you can reproduce the issue with a very small pdf, perhaps you can do binary validation on the resulting pdf? I can help get this through the finish line as I'll be needing it soon as well. |
BTW I tested your PR with the current master, it built fine. |
I'm going to pass this over to my colleague @dpausp as I'm out of the loop by now ... :) |
qt4 has been dropped #174634 |
Motivation for this change
At some point the build for wkhtmltopdf was switched to upstream Qt5. This is not intended to happen according to the
documentation that wkhtmltopdf itself has published:
When using the version currently available in NixOS we got a lot of complaints that wkhtmltopdf isn't working as expected any longer, specifically with weirdness around X integration, bad renderings, etc. I'm not sure why this was changed to Qt5.
However, I pulled in the original version of the package and updated it to the latest wkhtmltopdf. This works on my 19.03 based system but for some reason I get into a a weird situation on master where this fails to compile: qt4 itself builds fine, but wkhtmltopdf fails with:
I can see that QtWebkit is not included in the Qt4 output, and the build output seems to configure but not build it. I'm a bit confused by that and would like to:
a) discuss whether we would agree in general whether moving back to the patched qt4 version will be accepted
b) get some help figure out why this works on 19.03 but not master.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)