analogdevicesinc / libiio

A cross platform library for interfacing with local and remote Linux IIO devices
http://analogdevicesinc.github.io/libiio/
GNU Lesser General Public License v2.1
484 stars 313 forks source link

C++ API updated for new C-API #1112

Closed sternmull closed 8 months ago

sternmull commented 9 months ago

This updates iiopp.h for the latest changes of the C-API. It should fix issue #1046.

But be warned: I currently have no system with iio devices available for testing. So this is completely untested code!

The small example compiles and in general the code should be easy to read and review. I hope i didn't introduce any bugs. Please keep that in mind and review and test it carefully.

rgetz commented 9 months ago

since: the cpp bindings are off by default - https://github.com/analogdevicesinc/libiio/blob/61fdbe78c298570bbc0c9e9ec3ce1a7c1fa3742e/CMakeLists.txt#L651C1-L651C48

now things are working - we should turn them on for CI where we want to...

likely here for linux: https://github.com/analogdevicesinc/libiio/blob/61fdbe78c298570bbc0c9e9ec3ce1a7c1fa3742e/azure-pipelines.yml#L75

and here for mac: https://github.com/analogdevicesinc/libiio/blob/61fdbe78c298570bbc0c9e9ec3ce1a7c1fa3742e/azure-pipelines.yml#L337C4-L337C4

and here for windows: https://github.com/analogdevicesinc/libiio/blob/61fdbe78c298570bbc0c9e9ec3ce1a7c1fa3742e/CI/build_win.ps1#L14

and here for linux on ARM: https://github.com/analogdevicesinc/libiio/blob/61fdbe78c298570bbc0c9e9ec3ce1a7c1fa3742e/CI/azure/ci-ubuntu.sh

that way everything will get at least build tested on CI...

sternmull commented 9 months ago

Static code analysis passes. The Linux builds fail due to a missing C++ compiler. The windows build fails because boost is not installed. I don't know how to fix that. I think the Linux CI had a C++ compiler back when I added the bindings. Back then the C++ bindings where not activated for the windows CI-build.

Maybe someone who has experience with this CI-stuff can take a look at it?

rgetz commented 9 months ago

Centos fails since Target "iiopp-enum" requires the language dialect "CXX17" I think it's too old to support C++17... so turning that off makes sense. I think the same for Fedora34. Maybe @tfcollins knows more when he gets back in Jan.

If BOOST is a requirement - we should check for it in the CMAKE, and tell people to install it... something like:

  find_package(Boost QUIET NO_MODULE)

  if (Boost_FOUND)
    message(STATUS "Boost ${Boost_FIND_VERSION} found.")
    if (Boost_FIND_COMPONENTS)
      message(STATUS "Found Boost components:  ${Boost_FIND_COMPONENTS}")
    endif()
  else()
    message(SEND_ERROR "You need to install BOOST, or turn off C++ bindings")
  endif()

If you need specific modules - add them rather than NO_MODULE.

I think Travis can add BOOST to the docker image in Jan.

rgetz commented 9 months ago

So, boost is not a hard requirement for windows...

The code:

#if __cplusplus < 201703L
#include <boost/optional.hpp>
#else
#include <optional>
#endif

However - Visual Studio only fills out the __cplusplus var if you tell it to. (according to the doc, we need to add /Zc:__cplusplus compiler flag to enable the __cplusplus macro). I think that is the actual solution...

With maybe some more CMAKE to see if it needs to check for boost...

sternmull commented 9 months ago

So, boost is not a hard requirement for windows...

Yes. Actually it is not required with VS 2017 and later. Fixed that and a few minor problems (warnings that showed up with MS build).

rgetz commented 9 months ago

So, then we are left with:

LinuxBuilds fedora34:

-- The CXX compiler identification is unknown
CMake Error at bindings/cpp/CMakeLists.txt:3 (project):
  No CMAKE_CXX_COMPILER could be found.

debug : Install a C++ compiler in fedora34 image. I think @tfcollins needs to do this.

LinuxBuilds centos_7:

CMake Error in bindings/cpp/CMakeLists.txt:
  Target "iiopp-enum" requires the language dialect "CXX17" (with compiler
  extensions).  But the current compiler "GNU" does not support this, or
  CMake does not know the flags to enable it.
-- The C compiler identification is GNU 4.8.5

which was released June 23, 2015, before the cpp 2017 spec was written. Either figure out how to make it work without, and have BOOST installed, or (my suggestion) just turn it off in the azure-pipelines.yml file - there is a seperate line for Centos already...

pcercuei commented 9 months ago

@sternmull thanks a lot! I'll have a proper look next week.

tfcollins commented 8 months ago

Container updated https://github.com/sdgtt/Dockerfiles/commit/5ade2941e7a606bbfffcd3195d1a628af7611f64 Will be ready in 30 minutes

rgetz commented 8 months ago

@tfcollins - thanks - can someone with admin rights re-start the fedora build? or @sternmull can you squash things and push so things re-build?

pcercuei commented 8 months ago

All checks pass now.

sternmull commented 8 months ago

Done

pcercuei commented 8 months ago

Merged, thanks a lot!