Skip to content
This repository has been archived by the owner on May 16, 2024. It is now read-only.

[FIX] Switch to wkhtmltopdf GitHub URI #450

Merged
merged 1 commit into from
Jun 18, 2017
Merged

[FIX] Switch to wkhtmltopdf GitHub URI #450

merged 1 commit into from
Jun 18, 2017

Conversation

lasley
Copy link
Contributor

@lasley lasley commented Jun 16, 2017

Our CI is failing due to wkhtmltopdf/wkhtmltopdf#3518. I propose that we switch to a Github mirror temporarily, then we can switch back once I get the notification on the ticket.

An alternative, which may actually be better, is to host our wkhtmltopdf inside of MQT & not deal with the remote downloads. This isn't the first time we've had issues with wkhtmltopdf installations.

@lasley lasley mentioned this pull request Jun 16, 2017
Merged
@pedrobaeza
Copy link
Member

Look #449 instead

@lasley
Copy link
Contributor Author

lasley commented Jun 16, 2017

Thanks @pedrobaeza

I'm going to keep this open & update once the Github ticket is closed on their end. The solution in that ticket is to start pinning the binaries directly to the Github releases along with the source, so I will then change this to that URI instead for more stability.

@lasley lasley changed the title [FIX] Switch to wkhtmltopdf mirror temporarily [FIX] Switch to wkhtmltopdf GitHub URI Jun 16, 2017
@pedrobaeza
Copy link
Member

OK, good solution.

@pedrobaeza
Copy link
Member

And if GitHub goes down, maybe we can put the original one as mirror...

@lasley
Copy link
Contributor Author

lasley commented Jun 16, 2017

If Github is down, our builds are broken anyways 😉

@lasley lasley self-assigned this Jun 16, 2017
graingert
graingert reviewed Jun 16, 2017
View reviewed changes
travis/travis_install_nightly Outdated
@@ -83,7 +83,7 @@ if [ "$clone_result" != "0" ]; then
fi;

echo "Install webkit (wkhtmltopdf) patched"
(cd ${HOME}/maintainer-quality-tools/travis/ && wget -qO- https://downloads.wkhtmltopdf.org/0.12/0.12.3/wkhtmltox-0.12.3_linux-generic-amd64.tar.xz | tar -xJ --strip-components=2 wkhtmltox/bin/wkhtmltopdf)
(cd ${HOME}/maintainer-quality-tools/travis/ && wget -qO- https://github.com/graingert/wkhtmltopdf/releases/download/0.12.3/wkhtmltox-0.12.3_linux-generic-amd64.tar.xz | tar -xJ --strip-components=2 wkhtmltox/bin/wkhtmltopdf)
Copy link

Choose a reason for hiding this comment

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

please publish this to your own release page.

@lasley lasley force-pushed the bugfix/wkhtmltopdf-github branch from 6beaa36 to 4e174e1 Compare June 16, 2017 16:07
@lasley
Copy link
Contributor Author

lasley commented Jun 16, 2017

Looks like they were posted on the official releases page, so we're good to go from an official perspective it seems

@moylop260
Copy link
Contributor

FYI now wkhtmltopdf is using release from github, for latest version:

And I saw that they are discussing create the old ones too:
wkhtmltopdf/wkhtmltopdf#3518 (comment)

@lasley lasley force-pushed the bugfix/wkhtmltopdf-github branch from 4e174e1 to 0e925d3 Compare June 16, 2017 16:11
@lasley
Copy link
Contributor Author

lasley commented Jun 16, 2017

@moylop260 - change made in 0e925d3

graingert
graingert suggested changes Jun 16, 2017
View reviewed changes
travis/travis_install_nightly
@@ -84,7 +84,7 @@ fi;

if [ "${WKHTMLTOPDF_VERSION}" != "" ]; then
echo "Install webkit (wkhtmltopdf) patched version ${WKHTMLTOPDF_VERSION}"
(cd ${HOME}/maintainer-quality-tools/travis/ && wget -qO- -t 1 --timeout=240 https://downloads.wkhtmltopdf.org/0.12/${WKHTMLTOPDF_VERSION}/wkhtmltox-${WKHTMLTOPDF_VERSION}_linux-generic-amd64.tar.xz | tar -xJ --strip-components=2 wkhtmltox/bin/wkhtmltopdf)
(cd ${HOME}/maintainer-quality-tools/travis/ && wget -qO- -t 1 --timeout=240 https://github.com/wkhtmltopdf/wkhtmltopdf/releases/download/${WKHTMLTOPDF_VERSION}/wkhtmltox-${WKHTMLTOPDF_VERSION}_linux-generic-amd64.tar.xz | tar -xJ --strip-components=2 wkhtmltox/bin/wkhtmltopdf)
Copy link

Choose a reason for hiding this comment

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

remember to check the shasum!

moylop260
moylop260 approved these changes Jun 16, 2017
View reviewed changes
Copy link
Contributor

@moylop260 moylop260 left a comment

Choose a reason for hiding this comment

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

👍
Thanks!

@moylop260 moylop260 mentioned this pull request Jun 16, 2017
Merged
@lasley lasley force-pushed the bugfix/wkhtmltopdf-github branch 2 times, most recently from 761134a to 1436c18 Compare June 16, 2017 16:26
@lasley
Copy link
Contributor Author

lasley commented Jun 16, 2017

Oh wow I forgot how many builds there are. @moylop260 - do you know offhand which one would be attempting the wkhtmltox download?

graingert
graingert suggested changes Jun 16, 2017
View reviewed changes
travis/travis_install_nightly Outdated

cd ${HOME}/maintainer-quality-tools/travis/ &&
wkhtmltox=`wget -qO- -t 1 --timeout=240 https://github.com/wkhtmltopdf/wkhtmltopdf/releases/download/${WKHTMLTOPDF_VERSION}/wkhtmltox-${WKHTMLTOPDF_VERSION}_linux-generic-amd64.tar.xz` &&
sha256sum=`wget -qO- -t 1 --timeout=240 https://github.com/wkhtmltopdf/wkhtmltopdf/releases/download/${WKHTMLTOPDF_VERSION}/SHA256SUMS`
Copy link

@graingert graingert Jun 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

no. obviously you need to hardcode the SHASUM values in this codebase.

@moylop260
Copy link
Contributor

anyone without LINT_CHEK=1
e.g. first one

@dreispt
Copy link
Member

dreispt commented Jun 16, 2017

I like the idea of bundling wkhtmltopdf along with MQT.

moylop260
moylop260 reviewed Jun 16, 2017
View reviewed changes
travis/travis_install_nightly Outdated

cd ${HOME}/maintainer-quality-tools/travis/ &&
wkhtmltox=$(wget -qO- -t 1 --timeout=240 https://github.com/wkhtmltopdf/wkhtmltopdf/releases/download/${WKHTMLTOPDF_VERSION}/wkhtmltox-${WKHTMLTOPDF_VERSION}_linux-generic-amd64.tar.xz) &&
WKHTMLTOX_CHECKSUM='049b2cdec9a8254f0ef8ac273afaf54f7e25459a273e27189591edc7d7cf29db'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure of use this environment variable just for webkit.
I mean, we could use a wget_custom with a dictionary of checksum and url used from MQT.
But IMHO this is other feature (then other PR).

Could we use just the change of the URL like as main comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

For me, this PR is valid and enough with the following change: 0e925d3

@lasley
Copy link
Contributor Author

lasley commented Jun 16, 2017 •
edited
Loading

Ok so we'll keep the logical changesets separate & I will submit PRs for the other changes here. This is now just 0e925d3, which is simply the URI update.

@lasley lasley mentioned this pull request Jun 16, 2017
Open
1 task
@lasley lasley force-pushed the bugfix/wkhtmltopdf-github branch 2 times, most recently from 0e925d3 to d5ed3cc Compare June 16, 2017 20:39
@lasley lasley mentioned this pull request Jun 16, 2017
Open
@lasley
Copy link
Contributor Author

lasley commented Jun 16, 2017

@dreispt - I'm thinking if the releases themselves are hosted officially on Github, hosting them ourselves is probably not worth it. We're working under the same point of failure (GitHub), so I think that it's worth leaving the maintenance up to someone else and implementing the checksum validation in #452

pedrobaeza
pedrobaeza approved these changes Jun 18, 2017
View reviewed changes
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Merging this one for the builds that need wkhtmltopdf.

@pedrobaeza pedrobaeza merged commit 458311d into OCA:master Jun 18, 2017
@lasley lasley deleted the bugfix/wkhtmltopdf-github branch June 18, 2017 22:41
seb-elico added a commit to Elico-Corp/odoo-docker that referenced this pull request Jun 19, 2017
@seb-elico
Use GH release repo to download wkhtmltopdf
dffa9f6 
See:
OCA/maintainer-quality-tools#449
OCA/maintainer-quality-tools#450
OCA/maintainer-quality-tools#451
OCA/maintainer-quality-tools#452
seb-elico added a commit to Elico-Corp/odoo-docker that referenced this pull request Jun 19, 2017
@seb-elico
Use GH release repo to download wkhtmltopdf
9dac2c0 
See:
OCA/maintainer-quality-tools#449
OCA/maintainer-quality-tools#450
OCA/maintainer-quality-tools#451
OCA/maintainer-quality-tools#452
seb-elico added a commit to Elico-Corp/odoo-docker that referenced this pull request Jun 19, 2017
@seb-elico
Use GH release repo to download wkhtmltopdf
94e8f22 
See:
OCA/maintainer-quality-tools#449
OCA/maintainer-quality-tools#450
OCA/maintainer-quality-tools#451
OCA/maintainer-quality-tools#452
seb-elico added a commit to Elico-Corp/odoo-docker that referenced this pull request Jun 19, 2017
@seb-elico
Use GH release repo to download wkhtmltopdf
e83dd99 
See:
OCA/maintainer-quality-tools#449
OCA/maintainer-quality-tools#450
OCA/maintainer-quality-tools#451
OCA/maintainer-quality-tools#452
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@moylop260 moylop260 moylop260 approved these changes

@graingert graingert graingert requested changes

@pedrobaeza pedrobaeza pedrobaeza approved these changes

Assignees

@lasley lasley

Labels
needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lasley @pedrobaeza @moylop260 @dreispt @graingert