Convert package to a pure CMake package #285

moriarty · 2023-06-14T13:48:36Z

This PR started with a simple cherry-pick and has grown to include several other cherry-picks and review feedback.
It is to address #283

cherry-picking commit 4d5be00 from @cottsay
See: #209 (comment)

Author: Scott K Logan logans@cottsay.net
Date: Wed Jul 3 13:24:15 2019 -0700

Conflicts:
- CMakeLists.txt
- package.xml
- tests/CMakeLists.txt

@cottsay

cherry-picking commit 4d5be00 from @cottsay See: wjwwood#209 (comment) Author: Scott K Logan <logans@cottsay.net> Date: Wed Jul 3 13:24:15 2019 -0700 Conflicts: CMakeLists.txt package.xml tests/CMakeLists.txt Signed-off-by: Alex Moriarty <alex.moriarty@picknik.ai>

CMakeLists.txt

moriarty · 2023-06-14T14:09:52Z

In testing this locally it looks like I'll also need to cherry-pick

d8d1606

But this might just be due to newer system warnings

This couldn't be cherry-picked because but essentially the same change from the ros2 branch here: tylerjw@d8d1606 Fixes this error: ``` /usr/bin/ld: /home/alex/ros/h/robotiq/install/serial/lib/libserial.a(serial.cc.o): relocation R_X86_64_PC32 against symbol `_ZTVN6serial6SerialE' can not be used when making a shared object; recompile with -fPIC /usr/bin/ld: final link failed: bad value ``` Signed-off-by: Alex Moriarty <alex.moriarty@picknik.ai>

Use the pure CMake serial from: wjwwood/serial#285 Signed-off-by: Alex Moriarty <alex.moriarty@picknik.ai>

moriarty · 2023-06-14T15:36:11Z

I tried to cherry-pick d8d1606 from @tylerjw but it didn't apply cleanly so I just took the few lines directly in 380c4e4

However, I'll take a look through: #231 from @leamas and try to follow up on the this comment #231 (comment) to get a working shared version instead of just the static library

@wjwwood would you want the shared object version done in a follow up PR?

traversaro · 2023-06-14T15:43:09Z

CMakeLists.txt

@@ -48,6 +28,9 @@ endif()

 ## Add serial library
 add_library(${PROJECT_NAME} ${serial_SRCS})
+set_target_properties(${PROJECT_NAME} PROPERTIES
+    POSITION_INDEPENDENT_CODE ON)


Minor comment: this make it impossible to compile the library without -fPIC flag. An alternative that still defaults to enable -fPIC unless someone explicitly disable it is to add:

if (NOT DEFINED CMAKE_POSITION_INDEPENDENT_CODE) set(CMAKE_POSITION_INDEPENDENT_CODE ON) endif()

before the line add_library(${PROJECT_NAME} ${serial_SRCS})

Yes I agree this shouldn’t be merged as is with this it’s too invasive. I picked this out of the ros2 branch we’ve been using.

See comment above.

If you build the library as a shared library you don't need this.

I picked two commits from @leamas PR #231 (to set the so version) and then set BUILD_SHARED_LIBS ON.

Then tested that out of the box it works fine in a colcon workspace...
But if you do colcon build --cmake-args "-DBUILD_SHARED_LIBS=OFF" then you'll get back to ld errors

--- stderr: robotiq_driver /usr/bin/ld: /home/alex/ros/h/robotiq/install/serial/lib/libserial.a(unix.cc.o): warning: relocation against `_ZTVN6serial15SerialExceptionE' in read-only section `.text._ZN6serial15SerialExceptionD2Ev[_ZN6serial15SerialExceptionD5Ev]' /usr/bin/ld: /home/alex/ros/h/robotiq/install/serial/lib/libserial.a(serial.cc.o): relocation R_X86_64_PC32 against symbol `_ZTVN6serial6SerialE' can not be used when making a shared object; recompile with -fPIC /usr/bin/ld: final link failed: bad value

If I additionally add the suggestion from @traversaro then it seems to work in both cases:
colcon build --cmake-args "-DBUILD_SHARED_LIBS=OFF" will build a ./install/serial/lib/libserial.a which can be used without linking errors

@wjwwood any opinion or preference on this?

MarqRazz

Tested this change with the Robotiq gripper and it worked great.

Cmake made major changes in the 2.x -> 3.0 switch, keeping the 2.x compatiblity just isn't worth it. Since serial anyway doesn't build on versions before xenial, use xenial's cmake at 3.5 as baseline. Cherry-pick from PR wjwwood#231 Conflicts: CMakeLists.txt Author: Alec Leamas <leamas.alec@gmail.com> Date: Tue Sep 22 13:08:46 2020 +0200 Gbp-Pq: Name 0001-cmake-Use-cmake-3.5-add-project-setup.patch Signed-off-by: Alex Moriarty <alex.moriarty@picknik.ai>

Adding a so-version means defining an ABI level. This level is decoupled from the ordinary version, even a major version change doesn't necessarily mean that the so-version should change (and thus have all dependencies to be rebuilt). Adding the public header to clarify the setup. Note: cherry-pick from PR wjwwood#231 Conflicts: CMakeLists.txt Author: Alec Leamas <leamas.alec@gmail.com> Date: Tue Sep 22 13:28:04 2020 +0200 Gbp-Pq: Name 0002-cmake-Add-defined-so-version-and-public-header-to-li.patch Signed-off-by: Alex Moriarty <alex.moriarty@picknik.ai>

Default to building as a shared libary Signed-off-by: Alex Moriarty <alex.moriarty@picknik.ai>

- remove set BUILD_SHARED_LIBS ON - set CMAKE_POSITION_INDEPENDENT_CODE ON if undefined When setting BUILD_SHARED_LIBS if user set it to off linking would fail. Adding the suggetion from pull request review, which seems to work in both cases colcon build --cmake-args "-DBUILD_SHARED_LIBS=ON" colcon build --cmake-args "-DBUILD_SHARED_LIBS=OFF" Signed-off-by: Alex Moriarty <alex.moriarty@picknik.ai>

ChrisThrasher · 2023-06-15T14:41:38Z

CMakeLists.txt

@@ -1,31 +1,20 @@
-cmake_minimum_required(VERSION 2.8.3)
-project(serial)
+cmake_minimum_required(VERSION 3.5.0)


Why such an old version? I'm not sure what non-EOL platforms these days ship anything older than 3.16.

RHEL8 ships with 3.16, the oldest version of CMake in a distro currently supported by a tier 1 or 2 versions of ROS to give you more context. @ChrisThrasher, do you mind helping explain the benefits of using a newer version of CMake and provide some references in case @moriarty would like more information about this recommendation?

The main benefit is that it means you opt-in to many new and improved default behaviors (what CMake calls "policies") that have been added between version 3.5 and 3.16.

https://cmake.org/cmake/help/latest/manual/cmake-policies.7.html

On top of that, nobody is actually using 3.5 or testing with 3.5 so while we can claim this script doesn't use features added after 3.5 we can't be sure that unless we add a CI job that uses 3.5. CMake will let you use newer features even if your minimum version is very old.

I took 3.5 because I got a CMake warning that said something and suggested using at least 3.5

Also when I saw that warning, I just cherr-picked because it was on the other open PR which is what is being released into debian as libcxx-serial-dev : c9da89d but when I tried to use that package I had to add some define around the includes of serial (WIP testing here: PickNikRobotics/ros2_robotiq_gripper#22)

7b7f62b I've set this to 3.16

CMakeLists.txt

ChrisThrasher · 2023-06-15T14:42:05Z

CMakeLists.txt

+# General setup
+option(BUILD_SHARED_LIBS "Build using shared libraries" ON)
+
+set(PROJ_SOVERSION 1)


Why have this variable instead of just hardcoding this value a few lines down where it's used?

Similar to the recommendation to not use the PROJECT_NAME variable, biasing away from creating variables in cmake files makes them more boring and easier to read. CMake is a hard language to read and get correct and the more boring you can make it the nicer you are to yourself and others in the future.

One example where I've gone the other way from this recommendation though is in ros2_control with our use of the THIS_PACKAGE_INCLUDE_DEPENDS variable. You can find an example of that here: https://github.com/ros-controls/ros2_controllers/blob/f059022622e12115d006e0677ca2e312eed78858/joint_trajectory_controller/CMakeLists.txt#L8
The way I justify that variable is it greatly shortens the cmake file and cuts down on copy-paste errors in the dependencies that are specified for the project. I do not think your use of PROJ_SOVERSION is that case because it is just the number 1 and using the variable makes the cmake file longer (not to count the declaration of this variable) and adds indirection in every place where you use it. Is this a helpful explanation?

Makes sense. I will remove the PROJ_SOVERSION and just use 1.

all of the PROJECT_NAME variables come from cherry-pick from #231 and a later change in that PR makes it possible to have an alternative name for this library cxx-serial 5f1ab38 which is what @leamas released into the debian packages.

There are so many diverging forks of this library :'(

But I agree and will change PROJECT_NAME back to serial for readability in a separate commit so if @wjwwood or @leamas chime in to this discussion it could always be reverted.

PROJ_SOVERSION is actually crucial. It defines the binary compatibility level, and as such has been 1 so far. However, all serious packaging relies on a defined SOVERSION, without it all dependent packages will need to be recompiled for each and every change in the this lib, also the ones which are downwards compatible.

EDIT: I actually explained that in the commit, see below

That the library actually needs a new name is a reason, perhaps the only compelling one, to make a friendly fork. Forking is of course a bad thing, but perhaps the correct way to get out of the rabbit hole created by the too general serial/serial.h. It would make it easier to make big change while keeping the current lib as-is, for sure.

Issues could be moved to a fork without too much hassle, either in the GUI one-by-one or on the command line using a bulk transfer.

@leamas Nobody is asking to stop specifying the SOVERSION. We're saying that we don't need an extra variable to hold the value 1. That value can be hardcoded in the set_target_properties call where it's needed.

Keeping the name is about traceability. Put it this way: if you replace SOVERSION with a an explicit constant like 1 or 2 it will need a comment explaining what it's all about. Such needs for comments is a sure sign of bad design

Yes, cmake is a from time to time a hard language. However, this makes it even more important to stick to standard programming principles, one of which being to declare constants rather than using literal values. That is, SOVERSION is to be preferred to literal whatever.

Would to you propose a similar change to a C/C++ program? If you did, what do think the reaction would be?

How is

set_target_properties(${PROJECT_NAME} PROPERTIES VERSION ${PROJECT_VERSION} SOVERSION ${PROJ_SOVERSION} PUBLIC_HEADER "${serial_HEADERS}" )

better than

set_target_properties(${PROJECT_NAME} PROPERTIES VERSION ${PROJECT_VERSION} SOVERSION 1 PUBLIC_HEADER "${serial_HEADERS}" )

How is SOVERSION 1 unclear whatsoever? No comments are required to understand what's going on.

CMakeLists.txt

ChrisThrasher · 2023-06-15T14:43:54Z

CMakeLists.txt

+set(serial_HEADERS
+    include/serial/serial.h
+    include/serial/v8stdint.h
+)


Suggested change

set(serial_HEADERS

include/serial/serial.h

include/serial/v8stdint.h

)

You don't need to specify headers as source files if you don't want to.

cmake: Add defined so-version and public header to lib. Adding a so-version means defining an ABI level. This level is decoupled from the ordinary version, even a major version change doesn't necessarily mean that the so-version should change (and thus have all dependencies to be rebuilt). Adding the public header to clarify the setup. Gbp-Pq: Name 0002-cmake-Add-defined-so-version-and-public-header-to-li.patch

This was added here ed0e389 which I cherry-picked 61da1e2

v8stdint.h is actually problematic. It is now long time since I worked with this, but IIRC I think the situation is:

v8stdint.h is basically a band-aid for systems not having stdint.h available, notably windows.

The definitions in v8stdint.h clashes with other files on some systems.

That is, the correct solution would be to use cmake to probe for stdint.h and only include v8stdint.h if it's required. Needs to be double-checked, though. If it is indeed required anyway, cmake should probe for v8stdint.h and only include it if it's not available.

On Fedora, v8stdint.h is part of the v8 package.

Of course, this is not only about including v8stdint.h or not in the package. It is also about conditionalizing the source files so they include the correct file, stdint.h or v8stdint.h. I made a quick hack for this in the Debian package, but it probably needs polish before being merged into the upstream package here.

You don't need to specify headers as source files if you don't want to.

This is IMHO the wrong way to go. Recommended cmake best practice is certainly to include all used files in the project.

I have seen a numbner of cmake projects, none of without the headers in the source lists. In this case, I think that you need to come up with some trustworthy source recommending not to include the headers.

You have already started the list of functionality which breaks without a complete list of source files, including headers. I can assure you there is more, and that we currently not use it is not much of an argument.

BTW: how would you install headers not being part of the sources?

I think that you need to come up with some trustworthy source recommending not to include the headers.

I'd argue the burden of proof is on the person who wants to write more code.

You have already started the list of functionality which breaks without a complete list of source files

Nothing breaks for Visual Studio users. It's merely an inconvenience. Visual Studio users will also ask you use source_groups in spite of that not being strictly necessary.

how would you install headers not being part of the sources?

Same way as always (prior to FILE_SETs). You install the include directory or something similar. Adding headers as source files does not affect installation.

I simply don't have time for this, sorry. It's up to the maintainer to judge.

My experience has been that adding headers is not required at all. It does help some editors who want to list "files used in targets" without involving a compiler, but that's all. Whether or not it's recommended by CMake, I'm not sure about.

That being said, I don't care if they are included in this way. If it helps someone and they contribute it then I think it's fine.

Fair enough. But, as you said, cstdint.h is the better alternative to v8stdint.h making this to be serial.h only

ChrisThrasher · 2023-06-15T14:45:39Z

CMakeLists.txt

+    include/serial/serial.h
+    include/serial/v8stdint.h
+)
+# Build, link and install main library
 add_library(${PROJECT_NAME} ${serial_SRCS})


Suggested change

add_library(${PROJECT_NAME} ${serial_SRCS})

add_library(serial ${serial_SRCS})

It aids in readability and grepability if you simply spell out the variable name.

👍 to this recommendation as it makes the cmake file much easier to read and understand. One of the hardest parts of cmake is reading the config files.

I agree here, but I'll wait for @wjwwood to chime in on this PR in general.

I am concerned this PR gets too big and goes the way of any of the other open PRs

It's fine with me, but it deviates from what it common in the ROS ecosystem.

It's fine with me, but it deviates from what it common in the ROS ecosystem.

Not only the ROS ecosystem, but the whole cmake community I would say. For that reason I think this could be left as-is

@ChrisThrasher wrote

#285 (comment)

No. This should IMHO be done like in e1dabd8, taking into account whether to include v8stdint.h or not.

ChrisThrasher · 2023-06-15T14:51:16Z

cmake/serialConfig.cmake

@@ -0,0 +1,3 @@
+get_filename_component(SERIAL_CMAKE_DIR "${CMAKE_CURRENT_LIST_FILE}" PATH)
+set(SERIAL_INCLUDE_DIRS "${SERIAL_CMAKE_DIR}/../../../include")
+find_library(SERIAL_LIBRARIES serial PATHS ${SERIAL_CMAKE_DIR}/../../../lib/serial)


If we're adding a config module, we should write a config module that exports a target instead of doing the old school approach of setting some variables that downstream projects have to use. That means we need to create an export set for the library target and install that export set then this file becomes a one liner that will look something like this:

include(${CMAKE_CURRENT_LIST_DIR}/serial-targets.cmake)

@ChrisThrasher can you point to helpful documentation and examples on how to do this well?

https://github.com/ChrisThrasher/argon/blob/9a2982c5471f06ca1d97f8d2d8a1aab3055ba456/CMakeLists.txt#L29-L42

I'm biased but this installation code I wrote I quite like. It's goes above and beyond to be as robust and idiomatic as possible. You don't need to copy everything it does but it should do a good job laying our the basic steps.

Install all public headers. I do this by installing the entire include/ directory.

Create export set. This happens to only include a single library target but may include more if need be. It uses variables from the GNUInstallDirs module to ensure all files are installed in platform-appropriate locations that can be easily modified by the user should they want to install the library in a different location. I include the package name and version in the install tree but you can omit that.

Install said export set. This is where you can (and should) specify a namespace for all exported targets.

Create a version config file. This ensures that users can ask for a specific version and that will be checked against the version that is installed.

Install the config module and the version config file.

instead of doing the old school approach of setting some variables that downstream projects have to use.

This is not old-school. The normal usage pattern would be something like below, assuming that the serial library lives in a subdirectory

include(libs/serial) target_link_libraries(${PROJECT_NAME} PRIVATE wjserial::serial)

This would just require a one-liner in CMakeLists.txt defining a ALIAS target. It relies on cmake's support for transitive dependencies. Adding such a target would simplify documentation and overall usage a lot.

Yes, that's what I'm saying. We're in agreement. The variable-based approach is old school because targets are the modern way of doing things.

It aids in readability and grepability if you simply spell out the variable name.

This is actually a personal preference... I actually agree on the personal level, but let's face it: the use of ${PROJECT_NAME} is common enough in the cmake community to leave in place.

Don't mistake commonality for it being a good idea. Lots of bad practices are still popular but that should not be used as an argument in favor of perpetuating bad ideas.

tests/CMakeLists.txt

moriarty · 2023-06-15T15:06:54Z

FYI @tonybaltovski do you have time to give this a look over or a test? I know you left a comment here #209 (comment) that you're also depending on it and potentially going to fork it.

Bump cmake to oldest version currently used by tier 1 or 2 supported ROS platform. https://www.ros.org/reps/rep-2000.html#humble-hawksbill-may-2022-may-2027 https://www.ros.org/reps/rep-0003.html#noetic-ninjemys-may-2020-may-2025 Signed-off-by: Alex Moriarty <alex.moriarty@picknik.ai>

this is only used once and it makes it easier to read or grep for Signed-off-by: Alex Moriarty <alex.moriarty@picknik.ai>

Co-authored-by: Chris Thrasher <chrisjthrasher@gmail.com>

- Find and use GTest as imported target GTest::GTest - Note: - GTest::GTest is available from CMake 3.5 and depricated in 3.20 but still available in latest version. - GTest::gtest added in CMake 3.20 Signed-off-by: Alex Moriarty <alex.moriarty@picknik.ai>

wjwwood · 2023-06-21T21:37:42Z

CMakeLists.txt

+    DESTINATION include/serial)
+
+## Install CMake config
+install(FILES cmake/serialConfig.cmake
+    DESTINATION share/serial/cmake)
+
+
+## Install package.xml
+install(FILES package.xml
+    DESTINATION share/serial)


This might be a good time to change to exported targets, and also consider putting the headers in a separate subfolder, which we've been doing in ROS 2 for a while now to promote better header isolation in a merged install space.

I.e. we'd put the headers in include/serial/serial and the exported include flags would be like -I/path/to/include/serial with the code still doing #include "serial/serial.h".

wjwwood

Generally this lgtm, though it seems like we either need to address the lingering conversations with changes or new issues before merging.

Also, if you guys decide to fork, then maybe this can be done on the fork?

moriarty · 2023-06-21T21:52:08Z

Generally this lgtm, though it seems like we either need to address the lingering conversations with changes or new issues before merging.

I stopped editing this because I was not getting any feedback from you, and wanted to avoid what happened to all the previous attempts and pull requests to this repository.

If you’re okay with the lingering issues I’ll go ahead and make those changes.

Also, if you guys decide to fork, then maybe this can be done on the fork?

We were trying to avoid forming, and one reason to move to ros-drivers and not fork is because of how GitHub handles tracking forks and forks of forks. Issues opened on this repo would be lost on a fork, the link between forks of this repo and the forked repo would be lost or difficult to find.

Are you oppose to both having an additional maintainer on this project, and moving it to a common ros org… or just oppose #284 to moving it ?

wjwwood · 2023-06-21T22:22:52Z

I'm fine with the changes being proposed, but I think we should decide about moving/forking/add maintainers first.

I'll reply about moving vs forking on the #284 issue.

moriarty · 2023-06-26T16:02:32Z

@wjwwood or @leamas if we do fork to ros-drivers any input or suggestions on the name?

@leamas you've released this to debian as libcxx-serial-dev I would then try to get the rest of your changes in to keep from #231 previously I just skimmed it for what looked like the MVP pure CMake for the ROS 2 usecase

@wjwwood the serial name is two generic for the https://www.ros.org/reps/rep-0144.html ?

wjwwood · 2023-06-26T21:04:05Z

I answered the same question on the other issue, but yes it's too generic after the updates to rep-144.

I suggested ros_drivers_serial, but you guys can decide.

leamas · 2023-06-27T08:02:45Z

I don't really care either, but it would be nice to keep the ability to configure with an alternative name as of 5f1ab38. This is partly about Debian users, so they don't need to cope with a new library name.

It is also about me, changing the Debian package name is a convoluted process...

moriarty · 2023-06-27T15:55:31Z

I don't really care either, but it would be nice to keep the ability to configure with an alternative name as of 5f1ab38. This is partly about Debian users, so they don't need to cope with a new library name.

It is also about me, changing the Debian package name is a convoluted process...

Yes that makes sense don't want to make the debian package process any harder.

Would it make sense to take the cxx-serial name as the default name?
ROS packages are still released to debian as ros-${ROS_DISTRO}-package-name
eg: ros-iron-cxx-serial == libcxx-serial-dev ?

leamas · 2023-06-28T09:19:45Z

ROS is basically a mystery for me, haven't worked with it. That said, for me it would of course be convenient to use what I already use i. e., cxx-serial it it's ok with other stakeholders.

moriarty · 2023-07-06T19:49:15Z

Sorry have been out due to work/travel... Still out actually but I pushed added the src build of the ros2_robotiq_gripper to ros distro ros/rosdistro#37857

@wjwwood do you have permission to create a fork of this repo on the github.com/ros-drivers org?

wjwwood · 2023-07-06T19:54:20Z

Yes, I can do that, what name did we land on?

moriarty · 2023-07-06T19:57:46Z

I answered the same question on the other issue, but yes it's too generic after the updates to rep-144.

I suggested ros_drivers_serial, but you guys can decide.

There are already these two ros specific serial drivers:

https://github.com/ros-drivers/rosserial
https://github.com/ros-drivers/transport_drivers/tree/main/serial_driver

Since this is already released in debian by @leamas I would use the debian name, cxx-serial

wjwwood · 2023-07-06T20:07:53Z

For me, ros_drivers_serial reads as "The org ros_drivers's serial library", not "a ROS specific serial driver".

However cxx_serial is fine with me, but not that it should be cxx_serial and not cxx-serial in my opinion. The package name must use _ and I think to be consistent with other ROS packages the repository name should match the package name.

wjwwood · 2023-07-06T20:08:23Z

Let me know if cxx_serial is fine and I'll set that up. Also let me know who should be contributors to it initially.

tylerjw · 2023-07-06T20:24:16Z

Sounds good to me. @moriarty, @leamas, and yourself?

leamas · 2023-07-07T12:05:32Z

I can live with this, for sure. Naming the package cxx-serial even if the upstream is named cxx_serial should be fine. After all, I have named it cxx-serial for an upstream named serial...

EDIT: However, this only flies of we can keep 5f1ab38.

moriarty · 2023-07-07T15:37:19Z

I can live with this, for sure. Naming the package cxx-serial even if the upstream is named cxx_serial should be fine. After all, I have named it cxx-serial for an upstream named serial...

EDIT: However, this only flies of we can keep 5f1ab38.

Yes, I will go over your previous PR #231 and keep as much of your work, I really don't want to create a hard fork I'd rather the debian and he ros pkg be essentially the same.

EDIT: @leamas I'll aslo take your cmake related variable naming conventions which were heavily discussed above.

wjwwood · 2023-07-09T17:15:17Z

EDIT: However, this only flies of we can keep 5f1ab38.

We can't keep that because of the package.xml.in stuff. Also the CMake project name would need to be cxx_serial and not cxx-serial.

wjwwood · 2023-07-09T17:18:13Z

Ok, what I'm going to do is make a fork on my personal account, move any issues that should be moved, then move it to the ros-drivers once we've done that.

wjwwood · 2023-07-09T17:26:42Z

New repository is at https://github.com/wjwwood/cxx_serial, I invited you @moriarty and I'll try to triage the issues tonight and move the repository too. But I have to go for now.

moriarty · 2023-07-09T21:50:59Z

New repository is at https://github.com/wjwwood/cxx_serial, I invited you @moriarty and I'll try to triage the issues tonight and move the repository too. But I have to go for now.

Thanks!
I'll take a look tomorrow morning.

nagayev · 2023-08-31T09:25:59Z

@moriarty Hey, whats the PR status now?

moriarty · 2023-08-31T11:03:02Z

@moriarty Hey, whats the PR status now?

I plan to cherry-pick parts of it with the PR feedback from above to the new repository. Unfortunately priorities shifted but I still plan to get back to it

nagayev · 2023-09-04T20:40:01Z

@moriarty Can I help you?

moriarty commented Jun 14, 2023

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

moriarty added a commit to PickNikRobotics/ros2_robotiq_gripper that referenced this pull request Jun 14, 2023

[WIP] use pure CMake only serial

d00a00e

Use the pure CMake serial from: wjwwood/serial#285 Signed-off-by: Alex Moriarty <alex.moriarty@picknik.ai>

moriarty mentioned this pull request Jun 14, 2023

[WIP] use pure CMake only serial PickNikRobotics/ros2_robotiq_gripper#23

Draft

traversaro reviewed Jun 14, 2023

View reviewed changes

MarqRazz approved these changes Jun 14, 2023

View reviewed changes

leamas and others added 4 commits June 15, 2023 10:55

cmake: option BUILD_SHARED_LIBS ON

40d2950

Default to building as a shared libary Signed-off-by: Alex Moriarty <alex.moriarty@picknik.ai>

ChrisThrasher suggested changes Jun 15, 2023

View reviewed changes

moriarty mentioned this pull request Jun 16, 2023

Serial (Dependency Missing) [Meta Issue] PickNikRobotics/ros2_robotiq_gripper#21

Open

moriarty and others added 4 commits June 16, 2023 20:14

remove PROJ_SOVERSION variable

2a76e1a

this is only used once and it makes it easier to read or grep for Signed-off-by: Alex Moriarty <alex.moriarty@picknik.ai>

set project language to CXX

2528dcf

Co-authored-by: Chris Thrasher <chrisjthrasher@gmail.com>

wjwwood reviewed Jun 21, 2023

View reviewed changes

wjwwood mentioned this pull request Jun 21, 2023

Move serial to a ros-* org ? #284

Closed

	add_library(${PROJECT_NAME} ${serial_SRCS})
	add_library(serial ${serial_SRCS})

Convert package to a pure CMake package #285

Are you sure you want to change the base?

Convert package to a pure CMake package #285

Conversation

moriarty commented Jun 14, 2023 • edited Loading

moriarty commented Jun 14, 2023 • edited Loading

moriarty commented Jun 14, 2023

traversaro Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarqRazz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leamas Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leamas Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

ChrisThrasher Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leamas Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

ChrisThrasher Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisThrasher Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisThrasher Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisThrasher Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moriarty commented Jun 15, 2023

Choose a reason for hiding this comment

wjwwood left a comment

Choose a reason for hiding this comment

moriarty commented Jun 21, 2023

wjwwood commented Jun 21, 2023

moriarty commented Jun 26, 2023

wjwwood commented Jun 26, 2023

leamas commented Jun 27, 2023

moriarty commented Jun 27, 2023

leamas commented Jun 28, 2023

moriarty commented Jul 6, 2023

wjwwood commented Jul 6, 2023

moriarty commented Jul 6, 2023

wjwwood commented Jul 6, 2023

wjwwood commented Jul 6, 2023

tylerjw commented Jul 6, 2023

leamas commented Jul 7, 2023 • edited Loading

moriarty commented Jul 7, 2023 • edited Loading

wjwwood commented Jul 9, 2023

moriarty commented Jun 14, 2023 •

edited

Loading

moriarty commented Jun 14, 2023 •

edited

Loading

traversaro Jun 14, 2023 •

edited

Loading

leamas Jun 22, 2023 •

edited

Loading

leamas Jun 22, 2023 •

edited

Loading

ChrisThrasher Jun 22, 2023 •

edited

Loading

leamas Jun 22, 2023 •

edited

Loading

ChrisThrasher Jun 22, 2023 •

edited

Loading

ChrisThrasher Jun 15, 2023 •

edited

Loading

ChrisThrasher Jun 15, 2023 •

edited

Loading

ChrisThrasher Jun 22, 2023 •

edited

Loading

leamas commented Jul 7, 2023 •

edited

Loading

moriarty commented Jul 7, 2023 •

edited

Loading