lopsided98 / nix-ros-overlay

ROS overlay for the Nix package manager
Apache License 2.0
174 stars 68 forks source link

Fix sophus build errors for humble and iron #394

Open wentasah opened 2 months ago

wentasah commented 2 months ago

We apply the patch from the rolling version, which builds fine already now. The patch does not apply to foxy and noetic versions so we don't patch them. Foxy is EOL. If someone needs noetic version, more effort is needed.

Fixes #393

martiege commented 2 months ago

Nice! Do you have any idea why the noetic version doesn't work?

wentasah commented 2 months ago

Do you need noetic? I didn't look into the details. I assume the reason will be the same (i.e. newer compiler in nixpkgs generates more warnings). The solution would be also the same - remove -Werror from CMakeLists.txt, but that file looks differently in noetic and the patch doesn't apply. A different patch needs to be made for noetic.

martiege commented 2 months ago

Here is a quick patch doing that for noetic (on the upstream here: https://github.com/stonier/sophus/tree/release/1.1.x)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 26f0b3d..48fa083 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -16,11 +16,11 @@ set(CMAKE_CXX_STANDARD 11)
 IF("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
    SET(CMAKE_CXX_FLAGS_DEBUG  "-O0 -g")
    SET(CMAKE_CXX_FLAGS_RELEASE "-O3")
-   SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Werror -Wextra  -Wno-deprecated-register -Qunused-arguments -fcolor-diagnostics")
+   SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra  -Wno-deprecated-register -Qunused-arguments -fcolor-diagnostics")
 ELSEIF("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
    SET(CMAKE_CXX_FLAGS_DEBUG  "-O0 -g")
    SET(CMAKE_CXX_FLAGS_RELEASE "-O3")
-   SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Werror -Wextra -std=c++11 -Wno-deprecated-declarations -ftemplate-backtrace-limit=0")
+   SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -std=c++11 -Wno-deprecated-declarations -ftemplate-backtrace-limit=0")
    SET(CMAKE_CXX_FLAGS_COVERAGE "${CMAKE_CXX_FLAGS_DEBUG} --coverage -fno-inline -fno-inline-small-functions -fno-default-inline")
    SET(CMAKE_EXE_LINKER_FLAGS_COVERAGE "${CMAKE_EXE_LINKER_FLAGS_DEBUG} --coverage")
    SET(CMAKE_SHARED_LINKER_FLAGS_COVERAGE "${CMAKE_SHARED_LINKER_FLAGS_DEBUG} --coverage")
martiege commented 2 months ago

But if this is all that is needed to do, and the error seems to stem from building the tests and examples, wouldn't it be better to turn off those options?

martiege commented 2 months ago

Btw, while it is annoying that some of the ros-versions of sophus don't work, the library itself can be built and used with no issue. I just copied the derivation from nixpkgs unstable:

{ lib, stdenv, eigen, fmt, fetchFromGitHub, cmake }:
stdenv.mkDerivation rec {
  pname = "sophus";
  version = "1.22.10";

  src = fetchFromGitHub {
    owner = "strasdat";
    repo = "Sophus";
    rev = "${version}";
    hash = "sha256-TNuUoL9r1s+kGE4tCOGFGTDv1sLaHJDUKa6c9x41Z7w=";
  };

  nativeBuildInputs = [ cmake ];

  propagatedBuildInputs = [ eigen fmt ];

  cmakeFlags = [
    (lib.cmakeBool "BUILD_SOPHUS_TESTS" false)
    (lib.cmakeBool "BUILD_SOPHUS_EXAMPLES" false)
  ];
}

Then add it to the code I want like a normal library in cmake:

find_package(Sophus REQUIRED)
target_link_libraries(target Sophus::Sophus)
wentasah commented 2 months ago

But if this is all that is needed to do, and the error seems to stem from building the tests and examples, wouldn't it be better to turn off those options?

I'd prefer to build tests and examples, because it may allow discovering potential problems in the future. We just need to remove -Werror, because we use a newer compiler than upstream.

I've updated the commit to also patch Noetic. Now, all sophus versions build fine.