Skip to content
New issue

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

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

Already on GitHub? # to your account

Install Python module into site-packages #704

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

cjmayo
Copy link
Contributor

@cjmayo cjmayo commented Jan 14, 2024

Allows import manifold3d to work from the REPL or a script using the installed system interpreter.

Copy link

codecov bot commented Jan 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f407e43) 91.67% compared to head (7cfee4e) 91.67%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #704   +/-   ##
=======================================
  Coverage   91.67%   91.67%           
=======================================
  Files          37       37           
  Lines        4730     4730           
=======================================
  Hits         4336     4336           
  Misses        394      394           

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

@cjmayo
Copy link
Contributor Author

cjmayo commented Jan 14, 2024

OK, nix fails, maybe I am thinking like an downstream packager. Not sure is CMake has anything Python-ordinary-user friendly.

@pca006132
Copy link
Collaborator

I think the typical way is not using cmake to install the python package, but use pip.

@cjmayo
Copy link
Contributor Author

cjmayo commented Jan 15, 2024

I'm always going to build from source. Yes there is the manifold3d sdist on PyPI, an issue with 2.3.1 #706.

Advantages of installing from here:

  • The latest code is available without waiting for a release
  • Can use the same libmanifold with manifold3d and any dependent applications e.g. OpenSCAD.
  • Don't need to install scikit-build-core

Is there a point to installing into CMAKE_INSTALL_FULL_LIBDIR?

@pca006132
Copy link
Collaborator

Well you can install from source using pip build, this is how the sdist and bdist are created. The sdist issue you got is just a bug we should fix.

I think the current install path is needed for scikit-build-core to get the required library files, not sure about that.

@pca006132
Copy link
Collaborator

After looking at the code more carefully, I think this is fine but needs some tweaking for nix (which is normal, nix is... not a common distro).

diff --git a/flake.nix b/flake.nix
index af6d989..f728097 100644
--- a/flake.nix
+++ b/flake.nix
@@ -55,6 +55,10 @@
                 "-DFETCHCONTENT_SOURCE_DIR_THRUST=${thrust-src}"
                 "-DMANIFOLD_PAR=${pkgs.lib.strings.toUpper parallel-backend}"
               ];
+              prePatch = ''
+                substituteInPlace bindings/python/CMakeLists.txt \
+                  --replace 'DESTINATION ''${Python_SITEARCH}' 'DESTINATION "${placeholder "out"}/${pkgs.python3.sitePackages}"'
+              '';
               checkPhase = ''
                 cd test
                 ./manifold_test

Python_SITEARCH is not writeable by build_nix (tbb) job.
@cjmayo
Copy link
Contributor Author

cjmayo commented Jan 16, 2024

Updated and rebased with the suggested nix change (added to commit comment). All pass.
Thanks for investigating. nix looks interesting but alas would take some time to grasp.

And for the sdist fix too. I think it means people will also have a choice with releases of using cmake with the sdist for building libmanifold and manifold3d without needing to pull down anything else. I imagine down the line there will be some interest in building with system copies of the newer dependencies (I see nanobind has made it into ArchLinux and Debian) but I'm not going to try and guess there - it's not hard to patch things out in the CMakeLists's as it is anyway.

Copy link
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

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

Thanks!

@elalish elalish merged commit 9018b2a into elalish:master Jan 16, 2024
18 checks passed
@pca006132
Copy link
Collaborator

Yeah we try to allow users to use system packages if available, e.g. we just recently moved Clipper2 from submodule to query using pkg-config + FetchContent when not found. My main goal is to allow other packages (e.g. openscad) to use manifold without fighting with dependencies issue, e.g. they may want to directly use Clipper2 so they need to figure out which version of Clipper2 we are using.

For nanobind, I am not quite sure if we want to use system version or our vendored version. 1) It doesn't make much sense for distros to patch the underlying library file for nanobind when there is a vulnerability. There is no such dynamic library file, they have to build the package from source (I see this as the major reason to prefer system library and use dynamic linking). 2) From my experience, most distros have old packages that may lack something we need, especially for such young and active projects like nanobind. For example, we just recently updated nanobind to fix a serious bug. We can add version requirements to find_package, but it is cumbersome to maintain these version numbers and we will probably receive bug reports saying that our package can't be built under xxx distro...

I am still thinking about what is the best way of dealing with dependencies, we can open a discussion if you have more to add. Btw, feel free to open a PR to add your gentoo build to our CI to test it. This is the simplest way to make sure future versions doesn't break your build.

@cjmayo cjmayo deleted the site-packages branch January 18, 2024 19:44
cartesian-theatrics pushed a commit to SovereignShop/manifold that referenced this pull request Mar 11, 2024
Python_SITEARCH is not writeable by build_nix (tbb) job.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants