-
Notifications
You must be signed in to change notification settings - Fork 17
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
Install headers to include/${PROJECT_NAME} #46
Conversation
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
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 thing that I think should be changed, but this otherwise looks good to me.
URL_MD5 2464078985a73f9d298689d36061143f | ||
TIMEOUT 60 | ||
GIT_REPOSITORY https://github.com/yaml/libyaml.git | ||
GIT_TAG 2c891fc7a770e8ba2fec34fc6b545c672beb37e6 # 0.2.5 |
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'll point out that you can use an actual tag here (like 0.2.5
), which I think would be preferable.
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 not sure that's preferable. Currently a URL to a zip file and an MD5 sum are used. The MD5 sum makes sure the archive contains what we expect it to (limited by the ease of which it is to make an MD5 collision these days). If GIT_TAG
only says the human readable tag name, then a change to the tag upstream could break the build, or worse be a security issue. Using the sha hash solves that.
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.
We discussed this elsewhere, and we are going to switch to git hashes everywhere. So with that, I'll go ahead and approve.
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.
Looks good with green CI.
The patch is taken from this PR yaml/libyaml#244
This is in support of ros2/ros2#1150
The
externalproject_add(
changes are fromignition_math6_vendor
.