-
Notifications
You must be signed in to change notification settings - Fork 346
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
Add support to ubuntu 22.04 and 24.04 in arm64 #181
base: master
Are you sure you want to change the base?
Conversation
84d3fc4
to
2737d21
Compare
2737d21
to
8e80f6d
Compare
@@ -28,3 +28,4 @@ bin/wkhtmltopdf_ubuntu_20.04_amd64 | |||
bin/wkhtmltopdf_ubuntu_20.04_arm64 | |||
bin/wkhtmltopdf_ubuntu_21.10_amd64 | |||
bin/wkhtmltopdf_ubuntu_22.04_amd64 | |||
bin/wkhtmltopdf_ubuntu_22.04_arm64 |
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.
Did you mean this to be bin/wkhtmltopdf_ubuntu_24.04_arm64
? It's the same as the line above it.
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.
Did you mean this to be bin/wkhtmltopdf_ubuntu_24.04_arm64? It's the same as the line above it.
No, I think it is correct. The line above is ignoring the amd64
one whereas this is for arm64
. The file that should be ignored comes from https://github.com/zakird/wkhtmltopdf_binary_gem/pull/181/files#diff-5c40b9ebb9c6c8386d46fdafd1a1b8e564c5f6c6bc719676a2ce59337f8ea1cc when uncompressed (which happens when you run
wkhtmltopdf_binary_gem/bin/wkhtmltopdf
Line 89 in a29aa7d
Zlib::GzipReader.open("#{binary}.gz") { |gzip| file << gzip.read } |
Also, it's only ignoring this specific one because for 24.04, it would use the 22.04 binary
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.
@drsharp I missed it at first too, it's not the 22.04 part, it's the arm64
vs amd64
part.
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.
Aha! My old eyes. 👓 🤦
I see it now. ✔️
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'm glad it is resolved :)
Does this PR need any additional changes before it's ready to be merged @unixmonkey?
Thank you @chileung-b4b. I just used your addition on my Ubuntu 22.04 ARM installation and it worked great. I hope to see this merged in. |
Thank you @chileung-b4b! I was about to work in this direction, but your changes are working like a charm while testing some CI stuff on GitHub Actions with ARM. |
Updated description with instructions to check the added binary's hash |
The binary is the same for Ubuntu 22.04 and 24.04
Important
To check the binary is the same as the one from https://wkhtmltopdf.org/downloads.html before using and/or merging this branch, run the following: