search

wjwwood / serial

Cross-platform, Serial Port library written in C++
http://wjwwood.github.com/serial/
MIT License
2.16k stars 1.05k forks source link

Convert package to a pure CMake package #285

Open moriarty opened 1 year ago

moriarty commented 1 year ago

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: https://github.com/wjwwood/serial/pull/209#issuecomment-520018303

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

moriarty commented 1 year ago

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

https://github.com/wjwwood/serial/commit/d8d160678aa0b31cdf467c052b954fa287cc6cdf

But this might just be due to newer system warnings

moriarty commented 1 year ago

I tried to cherry-pick https://github.com/wjwwood/serial/commit/d8d160678aa0b31cdf467c052b954fa287cc6cdf 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: https://github.com/wjwwood/serial/pull/231 from @leamas and try to follow up on the this comment https://github.com/wjwwood/serial/pull/231#issuecomment-859110462 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?

moriarty commented 1 year ago

FYI @tonybaltovski do you have time to give this a look over or a test? I know you left a comment here https://github.com/wjwwood/serial/pull/209#issuecomment-1538507835 that you're also depending on it and potentially going to fork it.

moriarty commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

I don't really care either, but it would be nice to keep the ability to configure with an alternative name as of 5f1ab387b7dd7b. 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/ros/rosdistro/pull/37857

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

wjwwood commented 1 year ago

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

moriarty commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

leamas commented 1 year ago

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 5f1ab387b7.

moriarty commented 1 year ago

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 commented 1 year ago

EDIT: However, this only flies of we can keep https://github.com/wjwwood/serial/commit/5f1ab387b7dd7bc9f08bd5d8961a7e101a5dcca2.

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@moriarty Hey, whats the PR status now?

moriarty commented 1 year ago

@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 commented 1 year ago

@moriarty Can I help you?