Skip to content

nix: replace wkhtmltopdf with weasyprint#1895

Merged
eikek merged 3 commits intoeikek:masterfrom
VTimofeenko:nix-wkhtmltopdf-replace
Jan 16, 2023
Merged

nix: replace wkhtmltopdf with weasyprint#1895
eikek merged 3 commits intoeikek:masterfrom
VTimofeenko:nix-wkhtmltopdf-replace

Conversation

@VTimofeenko
Copy link
Copy Markdown
Contributor

Wkhtmltopdf is deemed insecure and requires a full build of qtwebengine. I am working on replacing wkhtmltopdf with weasyprint in Nix module definitions.

See #1873 for more background

This commit adds back the ability to run a development VM with docspell.
See updated documentation for examples.
@VTimofeenko VTimofeenko changed the title [draft] nix: replace wkhtmltopdf with weasyprint nix: replace wkhtmltopdf with weasyprint Dec 24, 2022
@VTimofeenko VTimofeenko marked this pull request as ready for review December 24, 2022 23:43

wkhtmlpdf = {
command = {
program = "${pkgs.wkhtmltopdf}/bin/wkhtmltopdf";
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think we should rather use the config option for weasyprint which has been added in the last release. I think I just missed to update the nix config values accordingly :( You can then switch to weasyprint by setting html-converter = weasyprint (see defaults)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got it. One thing to note - if pkgs.wkhtmltopdf is at all mentioned in the joex config - nix will complain about the insecure package. So, I think the changes should be:

  1. Add html-converter, default to weasyprint
  2. Add weasyprint set of options, enable by default
  3. Disable wkhtmltopdf by default, add some documentation note about the package being not secure

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yes that sounds great! I think we can just leave wkhtmltopdf empty or so, users could still set it if they want to.

@VTimofeenko
Copy link
Copy Markdown
Contributor Author

While trying to implement html-converter, I ended up with a config that does not mention wkhtmltopdf at all (see attached). However the conversion logs still mention wkhtmltopdf and conversion fails. Am I missing some additional setting?

@eikek
Copy link
Copy Markdown
Owner

eikek commented Jan 4, 2023

While trying to implement html-converter, I ended up with a config that does not mention wkhtmltopdf at all (see attached). However the conversion logs still mention wkhtmltopdf and conversion fails. Am I missing some additional setting?

* [job_log.txt](https://github.com/eikek/docspell/files/10328107/job_log.txt)

* [docspell-joex.conf](https://github.com/eikek/docspell/files/10328109/wcyxyviy7wz8436jk75a4xd13bwbcc1c-docspell-joex.txt)

Strange! I think then the config option which converter to use did not load correctly. 🤔 wkhtmltopdf should not be tried if weasyprint is set. But the config looks ok to me.

Edit: I just ran the json config through the config reader code and it produces the correct config object - weasyprint is set as converter. It's either some bug or perhaps it was not restarted?

@eikek
Copy link
Copy Markdown
Owner

eikek commented Jan 4, 2023

  • job_log.txt

Oh my…!? Just realized that you debugged some issue almost at new years eve 😱 hopefully you stopped very soon and had some better times then! 😄

@VTimofeenko
Copy link
Copy Markdown
Contributor Author

It's either some bug or perhaps it was not restarted?

I killed and redeployed the dev-vm to make sure. May this be a bug?

Oh my…!? Just realized that you debugged some issue almost at new years eve scream hopefully you stopped very soon and had some better times then! smile

Thanks :) I did have some down-time between the NY preparations and got some work done on this. Hope you had great holidays!

I have created a temporary branch with this config generator. To reproduce (if you have a machine with Nix):

# clone my temporary branch
git clone --depth=1 -b nix-wkhtmltopdf-replace-wip git@github.com:VTimofeenko/docspell.git

cd docspell/nix
# This will build a VM and bring up docspell
nix run '.#nixosConfigurations.dev-vm.config.system.build.vm'.
# To ssh into the machine and proxy the docspell port
ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -p 64022 -L64000:localhost:7880 root@localhost

after that open localhost:64000 in your browser, register and try to convert a txt file

@eikek
Copy link
Copy Markdown
Owner

eikek commented Jan 9, 2023

Thank you for all the details!! I'll try it out and report back. It must be some bug then, that I could not yet reproduce.

Edit: Oh 🤦🏼 I'm sorry! I overlooked the text file part. I have no idea why I didn't see it the other day, it is quite obvious! I need to fix this - currently text files are not working with weasyprint sadly 😞

Edit 2: For testing you can use a HTML file, this should be running weasyprint instead of wkhtmltopdf for the new config.

@eikek
Copy link
Copy Markdown
Owner

eikek commented Jan 15, 2023

Hi @VTimofeenko - I'm currently not sure if you wait for me? I think you have something done / in the doing regarding the weasyprint config option right? I'm sorry if I missed something 😅

@VTimofeenko VTimofeenko force-pushed the nix-wkhtmltopdf-replace branch from 092fff0 to 8fef423 Compare January 15, 2023 18:52
@VTimofeenko
Copy link
Copy Markdown
Contributor Author

Hey! Sorry for the delay - just got around to review my local changes. I emptied out the default command for wkhtmltopdf, added html-converter and weasyprint option. The dev-vm builds and converts an HTML file successfully and without complaining about qt being insecure

@eikek
Copy link
Copy Markdown
Owner

eikek commented Jan 16, 2023

Oh no, no need for sorry! I'm sorry if my wording implied this, was not intentional! I wasn't quite sure if there was something on my side. Thanks a lot!

@eikek eikek merged commit 7fa743f into eikek:master Jan 16, 2023
@VTimofeenko
Copy link
Copy Markdown
Contributor Author

All good :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants