-
Notifications
You must be signed in to change notification settings - Fork 6
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
Update xmllint download url. #37
base: latest
Are you sure you want to change the base?
Conversation
This makes it easier to change the xmllint download url in configuration rather than via code changes.
As a workaround for some host SSL issues we have rehosted these binaries on a host which does not use Let's Encrypt for HTTPS certificates.
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 left one nit to think about, but I'm going to approve anyway. You can decide whether to address it or not.
seven_zip_archive 'libxml2' do | ||
path 'C:\\xmllint' | ||
source 'https://www.zlatkovic.com/pub/libxml/64bit/libxml2-2.9.3-win32-x86_64.7z' | ||
source xmllint_url + 'libxml2-2.9.3-win32-x86_64.7z' |
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.
This is a minor nit, but using +
here seems a little fragile. If someone forgets to add the /
to the end of the download_url, this will produce an invalid URL. Does ruby/chef have the equivalent of '/'.join([a, b])
that Python has?
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.
It does
source xmllint_url + 'libxml2-2.9.3-win32-x86_64.7z' | |
source [xmllint_url, 'libxml2-2.9.3-win32-x86_64.7z'].join('/') |
and that would have the much less fatal effect of doubling the '/' if there is one present.
From a "keep it as obvious as possible" perspective I would probably opt for adding a leading slash to the file name and maintaining the concatenation rather than using a join. It has the additional, completely pointless, advantage of eliminating an additional array and string allocation (the array enclosing the url prefix and filename, and the literal '/'
).
source xmllint_url + 'libxml2-2.9.3-win32-x86_64.7z' | |
source xmllint_url + '/libxml2-2.9.3-win32-x86_64.7z' |
This will result in a doubled slash in the url in some cases but that should be benign to most web servers.
This reverts commit a5f9583. This was not a sufficient temporary workaround.
There are two commits here, one which refactors the downloads to use a single url path set via chef attribute and a second to change that attribute to point to the ROS download server (via OSUOSL FTP endpoint for proper https support) in order to work around outdated root CA info in the chef version used in these builds. Updating Chef is another more complete solution which will take time. This is meant to get us running in the meantime.