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.

Sign up for GitHub

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

Jump to bottom

Fix: build wkhtmltopdf with patched Qt4 #96379

Closed
wants to merge 3 commits into from
Closed

Fix: build wkhtmltopdf with patched Qt4 #96379

wants to merge 3 commits into from

Conversation

ctheune
Copy link
Contributor

@ctheune ctheune commented Aug 26, 2020

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:

In file included from ../lib/multipageloader_p.hh:24,
                 from ../lib/multipageloader.cc:21:
../lib/multipageloader.hh:30:10: fatal error: QWebPage: No such file or directory
   30 | #include <QWebPage>
      |          ^~~~~~~~~~
compilation terminated.
make[1]: *** [Makefile:434: multipageloader.o] Error 1
make[1]: *** Waiting for unfinished jobs....
In file included from ../lib/converter.cc:22:
../lib/converter_p.hh:27:10: fatal error: QWebSettings: No such file or directory
   27 | #include <QWebSettings>
      |          ^~~~~~~~~~~~~~
compilation terminated.
make[1]: *** [Makefile:448: converter.o] Error 1
In file included from /nix/store/ycrslxg21nzdiq9whvd0xwpv1whni595-qt-mod-4.8.7-7480f44/include/QtCore/QString:1,
                 from ../lib/logging.hh:25,
                 from ../lib/logging.cc:21:

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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.

@ctheune
Copy link
Contributor Author

ctheune commented Aug 26, 2020

Mentioning @orivej @lovek323 @Phreedom @sander as potential partners to discuss this.

@ctheune ctheune changed the title WIP: Fix: build wkhtmltopdf with patched Qt4 Draft: Fix: build wkhtmltopdf with patched Qt4 Aug 26, 2020
@ctheune ctheune marked this pull request as draft August 26, 2020 14:34
@ctheune ctheune changed the title Draft: Fix: build wkhtmltopdf with patched Qt4 Fix: build wkhtmltopdf with patched Qt4 Aug 26, 2020
@ofborg ofborg bot requested a review from jb55 August 26, 2020 15:00
@orivej
Copy link
Contributor

orivej commented Aug 31, 2020 •
edited

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 wkQt = qt4.overrideAttrs with wkQt = (qt4.override { buildWebkit = true; }).overrideAttrs, but you will have to fix the build of the old gstreamer first.

@orivej
Copy link
Contributor

orivej commented Sep 1, 2020

I have added a commit that fixes the build on master. It was failing due to gcc9 update. gstreamer turned out to be unnecessary.

@ctheune
Copy link
Contributor Author

ctheune commented Sep 15, 2020

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?

@ctheune ctheune marked this pull request as ready for review September 15, 2020 11:21
@ctheune ctheune mentioned this pull request Dec 1, 2020
Merged
@ctheune
Copy link
Contributor Author

ctheune commented Dec 1, 2020

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. :)

@jb55
Copy link
Contributor

jb55 commented Dec 1, 2020

don't need my blessing, I haven't been homesteading this for a long time now. happy to pass maintainership

@ctheune
Copy link
Contributor Author

ctheune commented Dec 1, 2020

@jb55 i really just need someone to push the button. I don't have commit rights, yet. :) maybe my colleague @dpausp wants those? not sure I can be trusted with commit rights as I do things to irregularly at the moment. ;)

@ctheune
Copy link
Contributor Author

ctheune commented Dec 2, 2020

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.

@ctheune ctheune marked this pull request as draft December 2, 2020 06:37
@veprbl veprbl mentioned this pull request Dec 2, 2020
Closed
@stale
Copy link

stale bot commented Jun 4, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 4, 2021
@elohmeier elohmeier mentioned this pull request Jul 6, 2021
Open
@grassit
Copy link

grassit commented Nov 27, 2021

@ctheune Thanks for bringing up the issue!
I was wondering how to "build wkhtmltopdf with patched Qt4"?
Is it to build from source https://github.com/wkhtmltopdf/wkhtmltopdf? If yes, what steps shall I follow? (I don't see makefile, so I guess I can't run make.)
Shall I install it into my home directory?
Shall I also build dependency qt from source and install it to my home directory?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 27, 2021
@ctheune
Copy link
Contributor Author

ctheune commented Dec 1, 2021

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

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 19, 2022
@kalbasit
Copy link
Member

@ctheune

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.

Have you / Do you need to update the PR with your changes?

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'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.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 13, 2022
@kalbasit kalbasit mentioned this pull request Jul 14, 2022
Merged
13 tasks
@kalbasit
Copy link
Member

BTW I tested your PR with the current master, it built fine.

@ctheune
Copy link
Contributor Author

ctheune commented Jul 21, 2022

I'm going to pass this over to my colleague @dpausp as I'm out of the loop by now ... :)

@Artturin
Copy link
Member

Artturin commented Sep 4, 2023

qt4 has been dropped #174634

@Artturin Artturin closed this Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jb55 jb55 Awaiting requested review from jb55

Assignees

@dpausp dpausp

Labels
6.topic: qt/kde 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@ctheune @orivej @jb55 @grassit @kalbasit @Artturin @veprbl @dpausp