Update uncrustify version to fix warnings on armhf #5
Conversation
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Full re-run after other PRs merged
|
|
The upstream PR has been merged. Instead of merging this PR I would suggest to rebase the vendor code to the current master of upstream instead. |
|
The vendor code logic is downloading a tarball of a specific release tag, currently, (https://github.com/ament/uncrustify_vendor/blob/master/CMakeLists.txt#L49) - are you sure we want to point it at master? My thinking was that we would wait until the next release, which will include this fix, and remove the patch then. |
Yes, that would be the intention. If you like to use the latest changes landed upstream using a commit hash from upstream sounds better than cherry-picking the change into our vendor package. During that process it will need to be made sure that all our tests pass with the updated version of |
|
OK - thanks for confirming. I will update it |
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
emersonknapp
changed the title
|
Here is a run doing a full build and a
|
|
@clalancette do you think we're good to merge now? Updated to the upstream version after that was merged. |
|
@emersonknapp I'm happy with this PR, but in order not to break CI we'll need to land ros2/rcutils#164 first/simultaneously. |
|
CI run against latest - only including this PR
|
|
All upstream changes have been merged, the above armhf build only has the unrelated warnings that are being fixed in ros2/demos#372 @clalancette good to merge? |
|
@clalancette ping |
| @@ -44,17 +44,20 @@ macro(build_uncrustify) | |||
| set(patch_exe patch) | |||
| endif() | |||
|
|
|||
| # Pinning to tip of master at the time of a desired bugfix | |||
| set(uncrustify_version 45b836cff040594994d364ad6fd3f04adc890a26) | |||
| set(uncrustify_version_hash 7cc32ad800c8d639bdf145e2a113a66b) | |||
| # Get uncrustify 0.68.1 | |||
There was a problem hiding this comment.
Keeping this comment unchanged is pretty confusing since the code doesn't use that specific version anymore.
Connected to ros2/ros2#721
Fixes these warnings https://ci.ros2.org/view/All/job/test_ci_linux-armhf/8/warnings23Result/package.575780676/
This patch mirrors just-opened uncrustify/uncrustify#2308 - once that change is bundled into a release, we can update the version we pull and drop the patch.
Signed-off-by: Emerson Knapp eknapp@amazon.com