-
Notifications
You must be signed in to change notification settings - Fork 420
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
Clean up install config files and ensure find_dependency is called as appropriate #452
Clean up install config files and ensure find_dependency is called as appropriate #452
Conversation
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.
Could you add a note in the CHANGELOG.md file?
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @traversaro)
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's a bug fix to 0.6.0. You will want to apply this to 0.6.1 ASAP otherwise Ubuntu packages are going to be semi-broken for two years.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @traversaro)
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.
Reviewed 3 of 3 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved
fcl-config.cmake.in, line 10 at r1 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
Partly tradition, but also makes for a clearer error message if the path is accidentally cleaned IMO.
Ok!
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.
Then let's go ahead and put it in a 0.6.1 section and we'll release it.
Reviewable status: complete! all files reviewed, all discussions resolved
You should probably cherry-pick #450 as well? |
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.
Reviewed 1 of 1 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved
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.
@sherm1 you want to put eyes on this as well?
Reviewable status: complete! all files reviewed, all discussions resolved
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.
- is there any way to test any of this? There are a lot of conditionals for which it would be reassuring to know we had gone through those paths.
- does there need to be some new documentation for the new features? For example, this introduces components "Development" and "Runtime". Does that need to be discussed in the README, or is this so standard that everyone will know what it does?
- This is some very nice ninja-level CMake code! Thanks, @jamiesnape.
Reviewed 3 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved
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.
- is there any way to test any of this? There are a lot of conditionals for which it would be reassuring to know we had gone through those paths.
Yes, if I refactor the test infrastructure (something I want to do, but not really something to add to this PR).
- does there need to be some new documentation for the new features? For example, this introduces components "Development" and "Runtime". Does that need to be discussed in the README, or is this so standard that everyone will know what it does?
Yes, it is standard.
Reviewable status: complete! all files reviewed, all discussions resolved
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.
Sounds like we should merge and tag release 0.6.1.
Reviewable status: complete! all files reviewed, all discussions resolved
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.
To be honest, I didn't follow that. My mind has a simpler model. We've tagged 0.6 several commits ago. We've added a few commits since then. Why not all of that becoming 0.6.1?
Reviewable status: complete! all files reviewed, all discussions resolved
So the first response to that would be why was the previous release tagged |
(don't squash the two commits) |
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.
Reviewed 2 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved
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.
Given the nature of the changes of the untested commit, I'm going to go ahead and merge.
Reviewable status: complete! all files reviewed, all discussions resolved
Closes #449. FYI @traversaro.
This change is