Downstream packaging patchset #231

leamas · 2020-09-24T15:53:39Z

EDIT: Rewritten docs after a rebase update.

An admittedly large patch set from the downstream Debian packaging:

Fix the missing so-version i. e., a defined ABI level (Packaging problems: soversion, deps #194)
Add some standard cmake configuration, PROJECT, PUBLIC_HEADER etc.
Add an official configuration switch ENABLE_TEST_PROGRAM to build the test program
Probe for doxygen, use it to build the docs if available.
Make the CATKIN_ENABLE_TESTING an official option, makes it visible in cmake help etc.
Don't include the ubiquitous v8stdint.h in package unless its' actually used.
Add an option to build using the alternative package name cxx-serial to avoid name conflicts when using the generic serial, by default off.
Fix the timing tests, now works on both MacOS and Linux.
ci updates:
- Add a travis build on Ubuntu Focal (as of today, all travis builds are broken). The existing, broken builds are left as-is after refactoring .travis.yml (Travis ci tests fails for linux and macOS #230). Update the xenial config to build a catkin package
- Add a appveyor.yml configuration building the win32 library
- Add a circleci configuration building Macos.

Closes: #194
Closes: #232
Closes: #230

These changes (besides the ci updates) will go into an upcoming Debian version. The current 1.2.1-3 contains a limited set.

Travis build logs Focal, Xenial
CircleCI build logs: MacOS, Focal

The patches are reasonably separate, cherry-picking should be possible.

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. Gbp-Pq: Name 0001-cmake-Use-cmake-3.5-add-project-setup.patch

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

Gbp-Pq: Name 0004-cmake-Make-test-program-depend-on-ENABLE_TEST_PROGRA.patch

Gbp-Pq: Name 0005-cmake-Use-doxygen-for-docs-if-found.patch

Gbp-Pq: Name 0006-cmake-Make-CATKIN_ENABLE_TESTING-an-offial-option.patch

Make sure that in cases v8stdint.h is not used it's not even included in the package. Installation of this file is problematic when packaged since the file is used also in other packages and leads to installation conflicts. Gbp-Pq: Name 0007-cmake-include-Make-use-of-v8stdint.h-conditional.patch

The name serial is problematic when packaging since it's basically too generic and easily generates name clashes. Add an option which builds the package as cxx-serial instead, by default off to not break existing usage. Gbp-Pq: Name 0008-cmake-Add-option-for-alternative-name-cxx-serial.patch

Compute actually elapsed time instead of assuming that usleep() sleeps for exactly the right time, which isn't guaranteed.

Although this patch better matches the state before the re-factoring, it doesn't mean that much since the build breaks already in the install stage. While on it, fix some yamllint nitpicks.

The existing targets for precise and osx breaks, both on dependencies. Seems to be about the 'rosdep install' command in the Makefile(?), which is beyond my current scope. Add a new target for Ubuntu focal which works after refactoring the .travis.yml file. Gbp-Pq: Name 0011-ci-.travis.yml-Refactor-add-working-focal-build.patch travis

leamas · 2020-09-29T11:31:36Z

Rebased & enhanced. See initial comment (edited)

wjwwood · 2021-06-10T22:05:30Z

Not sure I can take this in the current form, but I'll try to incorporate each of these things in the next version where I will be redoing CI (with GitHub Actions) and removing the catkin support in favor of pure CMake.

lp35 · 2022-11-15T12:59:24Z

CMakeLists.txt

-    )
-else()
-    # Otherwise normal call
+option(USE_CXX_SERIAL "build package name cxx-serial" OFF)


I would prefix all options using SERIAL_ so it can be properly regrouped in cmake GUI. What do you think?

Late to the party... but really no opinion about it, I never use the GUI...

lp35 · 2022-11-15T12:59:54Z

CMakeLists.txt

-    # Otherwise normal call
+option(USE_CXX_SERIAL "build package name cxx-serial" OFF)
+if (USE_CXX_SERIAL)
+    set(PKG_NAME cxx-serial)


Why not renaming this library as wjwserial? This remove useless clashes and make the cmakelist simpler

Because renaming the library basically means to fork it, and this is not what I intend to do.

lp35 · 2022-11-15T13:06:53Z

CMakeLists.txt

    add_subdirectory(tests)
 endif()
+
+if (DISABLE_CATKIN)


Not sure why this has been if'ed on CATKINS

So it can be built without the catkins dependency.

moriarty · 2023-06-13T18:09:42Z

include/serial/serial.h

+#ifdef HAVE_STDINT_H
+#include <stdint.h>
+#else
 #include <serial/v8stdint.h>
+#endif


To use the downstream libcxx-serial-dev I had to define HAVE_STDINT_H because the debian installs <serial/v8stdint.h> to /usr/include/v8stdint.h

https://packages.ubuntu.com/focal/amd64/libcxx-serial-dev/filelist

https://packages.ubuntu.com/jammy/amd64/libcxx-serial-dev/filelist

PickNikRobotics/ros2_robotiq_gripper#22 (comment)

Yes. A proper packaging would make sure that this compilation constant was set as defined by cmake. That is, this is actually a packaging bug.

Yes, this is a bug. This -D should be set automatically when building the package, as defined by cmake.

The simple solution would be to use target_add_definitions to add a compile flag to the target. The more elaborated approach would be to create a config.h file which would be included by all source files; this makes sense if there are more configuration which needs to be used in the sources.

Also, when this was written like 15 years ago this was needed, but now I think we could do without the v8stdint.h hack entirely.

Since this is C++, we can use <cstdint> and require C++11.

Yup, that' a better approach, agreed

moriarty · 2023-06-13T18:09:44Z

package.xml.in

+  <name>@PROJECT_NAME@</name>
+  <version>@PROJECT_VERSION@</version>


Does rosdep support package.xml -> package.xml.in when using rosdep to install all dependencies?

Not that I've been able to. If the project name is constant, then the maintainer just needs to keep this in sync with the CMakeLists, which is what most of the other packages do anyways.

As I understand this at a glance, rosdep is not aware of package.xml.in , it only uses package.xml which is updated at each cmake invocation

If the project name is constant, then the maintainer just needs to keep this in sync with the CMakeLists, which is what most of the other packages do anyways.

Indeed. However, the version is not constant.

You can't do this, sorry. The package.xml needs to exist before building.

It complicates the upstream maintenance, having especially the version in both CMakeLists and package.xml is not optimal.

It could be done the other way around: CMakeLists reads the version from package.xml. That would also be a consistent setup.

However, from a packaging perspective it doesn't really matter, so let's drop this for now

It could be done the other way around: CMakeLists reads the version from package.xml. That would also be a consistent setup.

Dart does exactly this: https://github.com/dartsim/dart/blob/main/CMakeLists.txt#L29 .

Yes. Even better would be to scan package.xml before the cmake project() invocation. If done this way the version could be set in project(...); cmake has some nice provisions to set up standard variables from this.

moriarty · 2023-06-13T18:11:17Z

tests/unix_serial_tests.cc

-#include <boost/bind.hpp>
-


I think this can be removed from the package.xml too

hm...

$ git grep bind.hpp tests/proof_of_concepts/tokenizer.cc:#include <boost/bind.hpp>

however, 2070f8a gets rid of this dep

The tests/proof_of_concepts directory seems to be dead code that's not being built

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 <[email protected]> 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 <[email protected]>

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 <[email protected]> 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 <[email protected]>

leamas mentioned this pull request Sep 24, 2020

tests: Works on travis, fails on my buildbox. #232

Open

leamas added 15 commits September 29, 2020 10:17

cmake: Make test program depend on ENABLE_TEST_PROGRAM.

742555a

Gbp-Pq: Name 0004-cmake-Make-test-program-depend-on-ENABLE_TEST_PROGRA.patch

cmake: Use doxygen for docs if found.

00cbd2c

Gbp-Pq: Name 0005-cmake-Use-doxygen-for-docs-if-found.patch

cmake: Make CATKIN_ENABLE_TESTING an official option.

c0e9292

Gbp-Pq: Name 0006-cmake-Make-CATKIN_ENABLE_TESTING-an-offial-option.patch

cmake: package.xml: Use version in PROJECT() call.

94743a6

cmake: Make catkin package optional

9662bf6

cmake: Move options to start, comments, cleanup.

13ac389

tests: Use elapsed time in timer tests (wjwwood#232).

2070f8a

Compute actually elapsed time instead of assuming that usleep() sleeps for exactly the right time, which isn't guaranteed.

ci: travis: Update to better match before refactoring.

a56c97e

Although this patch better matches the state before the re-factoring, it doesn't mean that much since the build breaks already in the install stage. While on it, fix some yamllint nitpicks.

ci: Add a circleci linux and macos builder.

ba8745b

ci: Add an appveyor windows build script.

7abad93

leamas force-pushed the pr branch from 6dd5782 to 7abad93 Compare September 29, 2020 11:30

leamas mentioned this pull request Sep 29, 2020

Travis ci tests fails for linux and macOS #230

Open

README.md: Update.

9dcca36

wjwwood added the next API change label Jun 10, 2021

lp35 reviewed Nov 15, 2022

View reviewed changes

This was referenced Jun 7, 2023

CMake [meta issue] #283

Open

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

Open

moriarty reviewed Jun 13, 2023

View reviewed changes

moriarty mentioned this pull request Jun 14, 2023

Convert package to a pure CMake package #285

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Downstream packaging patchset #231

Downstream packaging patchset #231

leamas commented Sep 24, 2020 •

edited

Loading

leamas commented Sep 29, 2020

wjwwood commented Jun 10, 2021

lp35 Nov 15, 2022

leamas Jun 22, 2023

lp35 Nov 15, 2022

leamas Jun 22, 2023

lp35 Nov 15, 2022

leamas Jun 22, 2023

moriarty Jun 13, 2023

leamas Jun 22, 2023

leamas Jun 22, 2023

wjwwood Jun 23, 2023

wjwwood Jun 23, 2023

leamas Jun 23, 2023

moriarty Jun 13, 2023

Ryanf55 Jun 17, 2023

leamas Jun 22, 2023

wjwwood Jun 23, 2023

leamas Jun 23, 2023

traversaro Jun 23, 2023

leamas Jun 23, 2023

moriarty Jun 13, 2023

leamas Jun 22, 2023

leamas Jun 26, 2023

moriarty Jul 11, 2023

		<name>@PROJECT_NAME@</name>
		<version>@PROJECT_VERSION@</version>

Downstream packaging patchset #231

Are you sure you want to change the base?

Downstream packaging patchset #231

Conversation

leamas commented Sep 24, 2020 • edited Loading

leamas commented Sep 29, 2020

wjwwood commented Jun 10, 2021

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

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

Choose a reason for hiding this comment

leamas commented Sep 24, 2020 •

edited

Loading