Skip to content
This repository has been archived by the owner on Jan 2, 2023. It is now read-only.

Allow to use long argument list #1768

Merged
merged 1 commit into from
Nov 22, 2016
Merged

Conversation

theirix
Copy link
Contributor

@theirix theirix commented Jun 10, 2014

Refactored entrance method to remove hardcoded argument list and buffer
sizes. Memory for arguments is allocated dynamically, lines from stdin
are read with growing buffer.

Rationale. Wkhtmltopdf have several hardcoded limits. They are quite large but we have already broken with large argument input files. This patch removes hardcoded limits and allows to use truly infinite argument/buffer sizes. Lack of POSIX getline function on several platforms requires to write a portable replacement fgets_large. Tested for FreeBSD, untested for Windows and Linux (should work).

Refactored entrance method to remove hardcoded argument list and buffer
sizes. Memory for arguments is allocated dynamically, lines from stdin
are read with growing buffer.
@ashkulz
Copy link
Member

ashkulz commented Jun 11, 2014

I am unlikely to merge these right now, as we are just about to release 0.12.1. Considering that you are using FreeBSD, any documentation updates on how you got it working there are more likely to be merged 😄

@theirix
Copy link
Contributor Author

theirix commented Jun 11, 2014

Sure, it can wait until a new version is released. I'll also try to test this patch on Linux.

I haven't tried to build and install wkhtmltopdf manually on FreeBSD. FreeBSD has a working port of wkhtmltopdf (converters/wkhtmltopdf) with a bunch of patches against of webkit and javascriptcore. Build is complicated due to two different checkouts - wkhtmltopdf and qt. Anyway port is well documented and maintainers could help to minimize source delta.

@ashkulz
Copy link
Member

ashkulz commented Jun 11, 2014

I know -- I've interacted with mm@ for the 0.12.0 version, and some of his suggestions are being implemented for 0.12.1. Just wanted to know if someone could check if the new build system works well on FreeBSD, so that development versions could also be built/tested on it 😄

@theirix
Copy link
Contributor Author

theirix commented Sep 3, 2015

Could you please tell what is the status of reviewing this pull request? Is it needed to explain changes more thoroughly?
You asked about build system on FreeBSD – FreeBSD port is active and there are new fresh versions so I think it is maintained well.

@ashkulz
Copy link
Member

ashkulz commented Sep 11, 2015

Sorry, I completely forgot about this. I haven't been getting time due to work pressure/health issues, will try to merge it next week as I'll have to test it for Windows and/or Linux. If you can confirm that it works on a platform other than FreeBSD, I'll merge it as-is.

@ashkulz ashkulz added this to the 0.12.3 milestone Sep 11, 2015
@theirix
Copy link
Contributor Author

theirix commented Sep 11, 2015

No problems, thank you for attention. I had not tested it for Linux so cannot guarantee correct work so it's better to test it.

@partychen
Copy link

can you accept the pullrequest, i met the same issues caused by the long parameters. I also try to modify this by myself.

@ashkulz ashkulz modified the milestones: future, 0.12.3 Apr 14, 2016
@theirix
Copy link
Contributor Author

theirix commented Jul 5, 2016

Sorry, I completely forgot about this. I haven't been getting time due to work pressure/health issues, will try to merge it next week as I'll have to test it for Windows and/or Linux. If you can confirm that it works on a platform other than FreeBSD, I'll merge it as-is.

Hello again!
Finally I have tested a latest wkhtmltopdf (master) with this patch at 64-bit Debian Jessie. It successfully compiled without warnings and launched with a very long parameters string. So it works at FreeBSD and Linux, cannot confirm Windows yet.

@ashkulz ashkulz merged commit 7d9f616 into wkhtmltopdf:master Nov 22, 2016
@ashkulz ashkulz added the Merged label Nov 22, 2016
@ashkulz ashkulz modified the milestones: 0.12.4, future Nov 22, 2016
@ashkulz
Copy link
Member

ashkulz commented Nov 22, 2016

Merged, thanks for the contribution!

@theirix
Copy link
Contributor Author

theirix commented Nov 22, 2016

Glad to hear it, thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

3 participants