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 to upstream (0.72.0) #13

Closed mjbogusz closed 2 years ago

mjbogusz commented 3 years ago

Is there a plan to update the bundled uncrustify to the upstream version? As it stands, the version packed here is older than the one in Ubuntu Focal repos (0.69.0).

Personally, I'm most interested in the following options added in 0.70.0:

Another possible solution would be to optionally depend on the system-provided uncrustify on *nixes and softlinking /opt/ros/<release>/bin/uncrustify to the /usr/bin/uncrustify binary - this way one could install another version and have e.g. ament_cmake_uncrustify use it automagically.

Note: Uncrustify (unfortunately) doesn't stick to semver and such an update could be a breaking change - some configuration files may need minor manual updating, e.g. with:

uncrustify --update-config-with-doc -c uncrustify.cfg -o uncrustify.cfg
nuclearsandwich commented 3 years ago

We've been updating the minimum version based on need.

The vendor package will actually check if there's an uncrustify binary already available and use it if the version is acceptable: https://github.com/ament/uncrustify_vendor/blob/master/CMakeLists.txt#L68-L88 so if you install your own uncrustify (including from Focal). In fact ros-foxy-uncrustify-vendor does exactly that.

If you wanted to contribute a PR updating the vendored version we can run CI with it to see what issues pop up.

mjbogusz commented 3 years ago

I've checked and indeed, the /opt/ros/<release>/bin/uncrustify binary doesn't exist on Foxy/Focal, but it does on Bionic/Eloquent.

My guess is it's dependent on the build setup, as it checks whether uncrustify is available during build-time. Where could I verify that suspicion? Also I wonder whether a script (e.g. /opt/ros/<release>/bin/uncrustify-vendor) checking the uncrustify version and selecting which binary to run at run-time would make any sense?

I'll try to push a PR soon - for now I'll target 0.72.0 but if it would cause too many issues even 0.70.1 will be an upgrade over the Ubuntu package.

nuclearsandwich commented 3 years ago

Where could I verify that suspicion?

In my previous message I sent you a link to the CMake logic where uncrustify is executed if found and the version information is extracted.

I wouldn't personally be interested in reviewing runtime script for this package. These vendor packages are only here to bridge gaps between what is available from the system package environment and what is needed by ROS. They should contain minimal build-time logic and no runtime logic except what is required provide build information to consumers of the vendored package.

Since ament_uncrustify looks for an uncrustify binary on PATH (https://github.com/ament/ament_lint/blob/master/ament_uncrustify/ament_uncrustify/main.py#L204-L209) you can provide your own uncrustify executable and as long as it is earlier in the path search order it will be used by ament_uncrustify.

mjbogusz commented 3 years ago

In my previous message I sent you a link to the CMake logic where uncrustify is executed if found and the version information is extracted.

I've meant the CI build configuration. By the looks of it the version from Ubuntu repo is installed - 0.66 for Bionic, 0.69 for Focal and that's why this package provides a binary for the former but not the latter.

Since ament_uncrustify looks for an uncrustify binary on PATH (https://github.com/ament/ament_lint/blob/master/ament_uncrustify/ament_uncrustify/main.py#L204-L209) you can provide your own uncrustify executable and as long as it is earlier in the path search order it will be used by ament_uncrustify.

Unfortunately sourcing ROS's setup.*sh (either from /opt or from a workspace) puts /opt/ros/<release>/bin at the beginning of PATH thus causing the ROS-provided uncrustify to overload the system-wide one.

I wouldn't personally be interested in reviewing runtime script for this package. These vendor packages are only here to bridge gaps between what is available from the system package environment and what is needed by ROS. They should contain minimal build-time logic and no runtime logic except what is required provide build information to consumers of the vendored package.

I see the reasoning and thinking about it it makes more sense that way. I've pushed a PR updating to v0.72 and with no other changes.

mjbogusz commented 2 years ago

Fixed by #27, didn't notice it earlier.