colcon / colcon-cargo

An extension for colcon-core to support Rust projects built with Cargo
http://colcon.readthedocs.io
Apache License 2.0
30 stars 20 forks source link

Reduce duplication between colcon-cargo and colcon-ros-cargo #21

Closed nnmm closed 2 years ago

nnmm commented 2 years ago

This tries to make colcon-cargo a better base class for colcon-ros-cargo, so that both packages have fewer combined lines.

As a concrete benefit, colcon-cargo now supports cleaning up the build directory.

I changed the installation location for binaries to be ${prefix}/bin, since I think that's a more standard location.

nnmm commented 2 years ago

I tested that the example from the README still works.

nnmm commented 2 years ago

@esteve There's also this PR. I'm not sure if it makes much sense for non-ROS packages to follow the https://www.ros.org/reps/rep-0122.html install space layout, but if it does, this would be a nice simplification.

esteve commented 2 years ago

@nnmm I have to look more into this, but a priori colcon-cargo must not depend / assume the same conventions as ament_cargo packages. colcon-cargo is meant to be for pure cargo packages, that don't depend on the ament build system or ROS at all.

nnmm commented 2 years ago

@esteve I changed this PR to no longer use cargo-ament-build. Now it's just a restructuring between this package and colcon-ros-cargo.

nnmm commented 2 years ago

Thanks! Unfortunately, CI seems to have a hiccup. Do you know what's missing @esteve?

esteve commented 2 years ago

@nnmm I've triggered CI again, but there seems to be a job stuck. I'll look into it later, but it doesn't seem to appear on the Github actions tab, though.

esteve commented 2 years ago

@nnmm I don't know where that job that is stuck comes from, I've submitted https://github.com/colcon/colcon-cargo/pull/23 which removes the GitHub actions to see if that test is from outside. It looks like it, there are no GitHub actions, but that job persists. I wonder if it's an organization-level webhook.

esteve commented 2 years ago

@nnmm it's really weird, seems to be coming from this old CI configuration:

https://github.com/colcon/colcon-cargo/blob/8d13687c13e88f729ecdf5d3c7f31e78f9513587/.github/workflows/ci.yml#L13-L14=

but I don't understand why it's being triggered now

esteve commented 2 years ago

@nnmm CI seems to be fixed now

nnmm commented 2 years ago

Cool! From renaming to main?