SICKAG / sick_safetyscanners_base

CPP (C++) Driver for SICK safety laser scanners
https://www.sick.com/de/en/opto-electronic-protective-devices/safety-laser-scanners/c/g187225
Apache License 2.0
12 stars 32 forks source link

Fix build for Ubuntu 24.04 #30

Closed ruffsl closed 3 months ago

ruffsl commented 4 months ago

When building from source for ROS 2 Jazzy via Ubuntu 24.04, one may encounter undefined uint8_t types:

Starting >>> sick_safetyscanners_base
--- stderr: sick_safetyscanners_base                             
In file included from /opt/overlay_ws/src/vendor/SICKAG/sick_safetyscanners_base/src/datastructure/DeviceStatus.cpp:35:
/opt/overlay_ws/src/vendor/SICKAG/sick_safetyscanners_base/include/sick_safetyscanners_base/datastructure/DeviceStatus.h:72:3: error: ‘uint8_t’ does not name a type
   72 |   uint8_t getDeviceStatus() const;
      |   ^~~~~~~
/opt/overlay_ws/src/vendor/SICKAG/sick_safetyscanners_base/include/sick_safetyscanners_base/datastructure/DeviceStatus.h:39:1: note: ‘uint8_t’ is defined in header ‘<cstdint>’; did you forget to ‘#include <cstdint>’?
   38 | #include <iostream>
  +++ |+#include <cstdint>
   39 | 

The difference in behavior between GCC 11.4.0 and GCC 13.2.0 may likely be due to changes in the C++ standard version used by default by the compiler, or changes in the implementation of the standard library. I think in C++11 and onwards, uint8_t is defined in the header. However, some older versions of GCC, or some configurations, might have allowed uint8_t to be used without explicitly including , possibly because another standard library header included indirectly?

In GCC 13.2.0, it seems that this is no longer the case, and we need to explicitly include to use uint8_t. But perhaps this is more in line with the C++ standard, which requires explicit inclusion of the headers defining the types used.

ruffsl commented 4 months ago

Oh, possible duplicate of this, although I didn't seem to need to add #include <string> for building locally.

cc @carlatcrown

carlatcrown commented 4 months ago

Oh, possible duplicate of this, although I didn't seem to need to add #include <string> for building locally.

cc @carlatcrown

Yes - same base cause - Noble updates :). We are iwyu so the was probably a cppcheck info warning (haven't double checked).

Not sure any owners are monitoring this repo currently - my PR was from Feb, might need to go through the company rep process for a prod etc.

ruffsl commented 4 months ago

@puck-fzi , please see this fix to help with your release into Jazzy:

puck-fzi commented 3 months ago

Closing since #25 was merged