Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix sophus build errors for humble and iron #394

Closed
wants to merge 1 commit into from
Closed

Fix sophus build errors for humble and iron #394

wants to merge 1 commit into from

Conversation

wentasah
Copy link
Contributor

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

@wentasah wentasah mentioned this pull request Apr 16, 2024
Open
@martiege
Copy link

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

@wentasah
Copy link
Contributor Author

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
Copy link

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
Copy link

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
Copy link

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
Fix sophus build errors
d1d2076 
Rolling version builds OK, other versions (noetic, humble, iron) fail.

For ROS 2, we apply the patch which is already rolling. The patch does
not apply to foxy and noetic versions. We ignore Foxy as it is EOL. We
patch noetic by removing explicitly removing "-Werror".

Fixes lopsided98#393
@wentasah
Copy link
Contributor Author

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.

@wentasah
Copy link
Contributor Author

This is no longer needed. Closing.

@wentasah wentasah closed this Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sophus doesn't build
2 participants
@wentasah @martiege