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.

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

cctag: 1.0.3 -> 1.0.4 #358972

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

cctag: 1.0.3 -> 1.0.4 #358972

wants to merge 1 commit into from

Conversation

emilazy
Copy link
Member

@emilazy emilazy commented Nov 25, 2024

Includes fixes for Boost 1.86.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Includes fixes for Boost 1.86.
Copy link
Contributor

@paparodeo paparodeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

darwin fails to build:


CMake Error at /nix/store/gjbd4dvxahrhpazz0mf005jqfhcd650d-boost-1.86.0-dev/lib/cmake/Boost-1.86.0/BoostConfig.cmake:141 (find_package):
  Could not find a package configuration file provided by "boost_math_c99"
  (requested version 1.86.0) with any of the following names:

    boost_math_c99Config.cmake
    boost_math_c99-config.cmake

@emilazy
Copy link
Member Author

emilazy commented Dec 29, 2024

Annoying. I’m afraid I have no idea why since it doesn’t seem like their build system is conditioning on Darwin at all and I don’t see anything in our Boost package that would leave components out on Darwin. I’ll try to take a look later.

@paparodeo
Copy link
Contributor

Annoying. I’m afraid I have no idea why since it doesn’t seem like their build system is conditioning on Darwin at all and I don’t see anything in our Boost package that would leave components out on Darwin. I’ll try to take a look later.

those directories are missing under darwin boost.dev/lib/cmake but they exist under linux for some reason.

@paparodeo
Copy link
Contributor

seems like darwin just doesn't need to link to libraries but has the headers? anyway, this fixes the build, checkPhase and the libraries.

fixes.diff
diff --git a/pkgs/development/libraries/cctag/cmake-no-apple-rpath.diff b/pkgs/development/libraries/cctag/cmake-no-apple-rpath.diff
new file mode 100644
index 000000000000..f878ba26a180
--- /dev/null
+++ b/pkgs/development/libraries/cctag/cmake-no-apple-rpath.diff
@@ -0,0 +1,25 @@
+diff --git a/CMakeLists.txt b/CMakeLists.txt
+index d0e35b6..fc19477 100644
+--- a/CMakeLists.txt
++++ b/CMakeLists.txt
+@@ -96,20 +96,6 @@ endif()
+ # set the path where we can find the findXXX.cmake
+ set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${PROJECT_SOURCE_DIR}/cmake")
+ 
+-if(APPLE)
+-
+-  # avoid the cmake policy warning about @rpath in MacOSX
+-  cmake_policy(SET CMP0042 NEW)
+-
+-  SET(CMAKE_MACOSX_RPATH TRUE) # initialize the MACOSX_RPATH property on all targets
+-  SET(CMAKE_SKIP_BUILD_RPATH  FALSE) # don't skip the full RPATH for the build tree
+-  # SET(CMAKE_BUILD_WITH_INSTALL_RPATH FALSE) # when building, don't use the install RPATH already
+-  SET(CMAKE_BUILD_WITH_INSTALL_RPATH TRUE) # when building, use the install RPATH already
+-                                           # probably not needed
+-  # SET(CMAKE_INSTALL_RPATH "") # the RPATH to be used when installing
+-  SET(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE) # LC_RPATH for CUDA and OpenCV etc written into executable
+-endif(APPLE)
+-
+ # FIND BOOST
+ set(BOOST_REQUIRED_COMPONENTS "atomic;chrono;date_time;filesystem;program_options;serialization;system;thread;timer;math_c99")
+ if(WIN32)
diff --git a/pkgs/development/libraries/cctag/default.nix b/pkgs/development/libraries/cctag/default.nix
index 3259822d1601..40986061a56e 100644
--- a/pkgs/development/libraries/cctag/default.nix
+++ b/pkgs/development/libraries/cctag/default.nix
@@ -15,7 +15,7 @@ stdenv.mkDerivation rec {
   pname = "cctag";
   version = "1.0.4";
 
-  outputs = [ "lib" "dev" "out" ];
+  outputs = [ "out" "dev" ];
 
   src = fetchFromGitHub {
     owner = "alicevision";
@@ -36,8 +36,17 @@ stdenv.mkDerivation rec {
 
   patches = [
     ./cmake-install-include-dir.patch
+
+    # remove custom rpath settings as they break linking to the lib
+    ./cmake-no-apple-rpath.diff
   ];
 
+  # darwin boost doesn't have math_c99 libraries
+  postPatch = lib.optionalString stdenv.hostPlatform.isDarwin ''
+    substituteInPlace CMakeLists.txt --replace-warn ";math_c99" ""
+    substituteInPlace src/CMakeLists.txt --replace-warn "Boost::math_c99" ""
+  '';
+
   nativeBuildInputs = [
     cmake
   ];
@@ -52,8 +61,7 @@ stdenv.mkDerivation rec {
     opencv.cxxdev
   ];
 
-  # Tests are broken on Darwin (linking issue)
-  doCheck = !stdenv.hostPlatform.isDarwin;
+  doCheck = true;
 
   meta = with lib; {
     description = "Detection of CCTag markers made up of concentric circles";

Copy link
Contributor

@tobim tobim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to merge after @paparodeo's patch is applied.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 22, 2025
@Scrumplex Scrumplex added the awaiting_changes (old Marvin label, do not use) label Jan 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants