Clemapfel / jluna

Julia Wrapper for C++ with Focus on Safety, Elegance, and Ease of Use
https://clemens-cords.com/jluna
MIT License
246 stars 14 forks source link

various fixes for cmake based find_package #10

Closed robertu94 closed 2 years ago

robertu94 commented 2 years ago

@Clemapfel Thanks for your great work on Jluna! I've been waiting for a package like this. Here are a few changes that I made locally that made it easier to use for my uses cases. Please feel free to merge if these are useful for you.

robertu94 commented 2 years ago

I don't have an easy way to test on windows, but I suspect the changes that I made here work on Windows too.

Clemapfel commented 2 years ago

Hi! Thank you for contributing!

I suck at cmake, so I'm sorry if I didn't understand it correctly, but in your version we install jluna and the c_adapter in the global, OS install dir so that downstream project can just include it from there? I don't know how comfortable I am with that being the default cmake behavior, I'd much rather have the shared library be generated locally, though your version has the advantage that, whenever jluna (the git repo folder) is moved, the user does not have to recompile to set RESOURCE_PATH again.

I also disagree with jluna being shipped with the build type set to anything but release, there are a non-trivial amount of c-assertions everywhere that would decrease performance (because they are disabled when NDEBUG isn't set`. If I push something to master and draft a release, jluna should be assumed to be bug-free and thus there is no need for jluna itself being set to DEBUG.

Lastly, building your fork (after rebasing it on the current master, I updated it very recently), compilation succeeds but jl_init fails with:

/home/clem/Desktop/jluna/cmake-build-debug/JLUNA_TEST
ERROR: could not load library "/lib/x86_64-linux-gnu/../bin/../lib/x86_64-linux-gnu/julia/sys.so"
/lib/x86_64-linux-gnu/../bin/../lib/x86_64-linux-gnu/julia/sys.so: cannot open shared object file: No such file or directory

Process finished with exit code 1

This is because Julia isn't in my gnu install folder, that was the entire reason why I wrote the cmake to detect it manually, it's the only way to account for cases like that and julia has the really nice Sys.BINDIR variable which makes this process automated.

I do very much like your cleaner way of setting RESOURCE_PATH (now: BUILD_RESOURCE_DIR) and using CTest instead of asking the user to do it themselves in the installation manual. I'll give myself a few days to think about the design decision, but I'll definitely merge those parts of the PR.

Also, despite my deep hatred for it, I do think a windows port is on the near horizon. I had too many people ask for it so any change to the architecture needs to be platform-independent, which the current master is more than your fork if I understand correctly.

robertu94 commented 2 years ago

I suck at cmake, so I'm sorry if I didn't understand it correctly, but in your version we install jluna and the c_adapter in the global, OS install dir so that downstream project can just include it from there?

That's the rough idea. However, in my case I would install it via a package manager like spack which puts each package in it's own "prefix" ~/git/spack/opt/<some_hash>, spack (or other package managers), then set CMAKE_PREFIX_PATH to find where it was installed if it wasn't put in /usr/

The exact directory that everything is installed into is user specifiable using CMAKE_INSTALL_PREFIX

I also disagree with jluna being shipped with the build type set to anything but release

The default is currently release if nothing else is set. What this does is if the user requests it via setting CMAKE_BUILD_TYPE=RelWithDebInfo build a release version that also has debugging symbols (basically -O2 -g), and if they really want to accept the performance costs, they can set it to Debug without having to edit the CMakeLists.txt

Lastly, building your fork (after rebasing it on the current master, I updated it very recently) ... compilation succeeds but jl_init fails with

Opps, sorry about that. I'd have to test that some more. Do you mind sharing what OS and where Julia is installed so I can test my patch?

[supporting Windows ...] which the current master is more than your fork if I understand correctly.

That certainly wasn't intentional if there were any problems. I think I missed merging one commit you made this afternoon. Cmake functions like find_library and find_path should know about microsoft-isms (like the different extensions for shared objects), but I've never written anything more than hello world on Windows so I'd have to test.

Clemapfel commented 2 years ago

moved cmake update and windows support to #13