search

ament / uncrustify_vendor

CMake shim over the uncrustify library: https://github.com/uncrustify/uncrustify
Apache License 2.0
0 stars 9 forks source link

Update uncrustify version to fix warnings on armhf #5

Closed emersonknapp closed 5 years ago

emersonknapp commented 5 years ago

Connected to https://github.com/ros2/ros2/issues/721

Fixes these warnings https://ci.ros2.org/view/All/job/test_ci_linux-armhf/8/warnings23Result/package.575780676/

This patch mirrors just-opened https://github.com/uncrustify/uncrustify/pull/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

emersonknapp commented 5 years ago

Full re-run after other PRs merged

dirk-thomas commented 5 years ago

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.

emersonknapp commented 5 years ago

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.

dirk-thomas commented 5 years ago

are you sure we want to point it at master?

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 uncrustify.

emersonknapp commented 5 years ago

OK - thanks for confirming. I will update it

emersonknapp commented 5 years ago

Here is a run doing a full build and a --mixin linters-only test (with this PR and the two above Connected ones fixing formatting for latest uncrustify)

emersonknapp commented 5 years ago

@clalancette do you think we're good to merge now? Updated to the upstream version after that was merged.

clalancette commented 5 years ago

@emersonknapp I'm happy with this PR, but in order not to break CI we'll need to land https://github.com/ros2/rcutils/pull/164 first/simultaneously.

emersonknapp commented 5 years ago

CI run against latest - only including this PR

emersonknapp commented 5 years ago

All upstream changes have been merged, the above armhf build only has the unrelated warnings that are being fixed in https://github.com/ros2/demos/pull/372

@clalancette good to merge?

emersonknapp commented 5 years ago

@clalancette ping