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

Use system nanobind if installed #880

Merged
merged 6 commits into from
Aug 3, 2024
Merged

Conversation

cjmayo
Copy link
Contributor

@cjmayo cjmayo commented Jul 30, 2024

I think I saw mention of supporting system nanobind in one of the discussions, it is becoming a thing now.

This uses the recommended approach - with additional version checking.

This still falls back to the third_party directory for now, but obviously that is easily replaced. For pyproject.toml users automatically fetching nanobind is supported.

Copy link

codecov bot commented Jul 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.08%. Comparing base (d437097) to head (be2846e).
Report is 67 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #880      +/-   ##
==========================================
- Coverage   91.84%   90.08%   -1.77%     
==========================================
  Files          37       64      +27     
  Lines        4976     9184    +4208     
  Branches        0      956     +956     
==========================================
+ Hits         4570     8273    +3703     
- Misses        406      911     +505     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@elalish
Copy link
Owner

elalish commented Jul 30, 2024

I think in general that fewer submodules would be great. Any reason not to remove it from third_party? I'll defer to @pca006132 on approving this one.

@pca006132
Copy link
Collaborator

It seems that when building using python build, it automatically checks nanobind version and fail when it is not installed, even when we have a submodule.

Re. submodule, I think we can switch to FetchContent as well.

@pca006132
Copy link
Collaborator

It seems that when building using python build, it automatically checks nanobind version and fail when it is not installed, even when we have a submodule.

Or maybe this is just the case when the build environment is not connected to the internet? We can add the package to nix though, it is already packaged.

diff --git a/flake.nix b/flake.nix
index 05677219..3ea2dc1c 100644
--- a/flake.nix
+++ b/flake.nix
@@ -123,6 +123,7 @@
                 tbb
                 glm
                 clipper2
+                nanobind
               ];
               nativeBuildInputs = with pkgs; [
                 cmake

@cjmayo cjmayo force-pushed the system-nanobind branch from e6ff892 to 85e27a9 Compare July 31, 2024 18:24
@cjmayo
Copy link
Contributor Author

cjmayo commented Jul 31, 2024

flake.nix update worked.

I'll have a look at FetchContent.

@cjmayo
Copy link
Contributor Author

cjmayo commented Aug 1, 2024

Now using FetchContent instead of a submodule/

@@ -14,7 +14,25 @@

project(python)

add_subdirectory(third_party)
execute_process(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for nix, there is no python nanobind package. I think we can do find_package(nanobind 1.8.0 CONFIG REQUIRED), and if nanobind_FOUND is not defined we can do the fetch content?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is already trying to FetchContent:

manifold-tbb> -- nanobind not found, downloading from source
manifold-tbb> CMake Error at /nix/store/q1nssraba326p2kp6627hldd2bhg254c-cmake-3.29.2/share/cmake-3.29/Modules/ExternalProject.cmake:2945 (message):
manifold-tbb>   error: could not find git for clone of nanobind-populate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like this works:

@@ -35,7 +35,7 @@
                 cmake
                 ninja
                 (python3.withPackages
-                  (ps: with ps; [ trimesh pytest ]))
+                  (ps: with ps; [ nanobind trimesh pytest ]))
                 gtest
                 pkg-config
               ]) ++ build-tools;

but build_nix_python is still broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK:

@@ -133,6 +133,7 @@
                 pkg-config
               ];
               checkInputs = [
+                nanobind
                 trimesh
                 pytest
               ];

@pca006132 pca006132 merged commit a37d59e into elalish:master Aug 3, 2024
21 checks passed
@cjmayo cjmayo deleted the system-nanobind branch August 14, 2024 18:22
@elalish elalish mentioned this pull request Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants