various fixes for cmake based find_package #10
Conversation
+ modified #include directives so that they work outside of installed as a subtree of the project + modified how jluna.jl is found so that they can be found if they are installed or just simply built, by default, it prefers the local version from the installed versions. I would have loved to use c++ std::format here rather than snprintf, but since the supported compilers don't support it I can't use it here. + modified how libjluna_c_adapter.so is found to use the dynamic linker instead of explicit path passing which improves relocatablity + rather than always forcing a Release build, allow the user to set it to each of the CMake defaults + include CTest to make compiling the unit tests optional in the standard way + include GNUInstallDirs which was not provided before + use find_library() to locate libjulia.so to make it more robust to different OS layouts + use find_path() to locate julia.h to make it more robust to different OS layouts + set the target_include_directories and target_link_libraries correctly so that downstream consumers can simply ```cmake find_package(jluna) target_link_libraries(foo PRIVATE jluna::jluna) ``` and everything works + simplified the logic to set the library name. Note that using `global` in the module is safe because `global`s in julia are local to the [module they are defined in](https://docs.julialang.org/en/v1/manual/modules/)
|
I don't have an easy way to test on windows, but I suspect the changes that I made here work on Windows too. |
|
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 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 Lastly, building your fork (after rebasing it on the current master, I updated it very recently), compilation succeeds but 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 I do very much like your cleaner way of setting 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. |
| const char* julia_lib_load_template = R"julia( | ||
| begin | ||
| local dev_path = "%s/include/jluna.jl" | ||
| local prod_path = "%s/jluna/jluna.jl" | ||
| if isfile(dev_path) | ||
| include(dev_path) | ||
| else isfile(prod_path) | ||
| include(prod_path) | ||
| end | ||
| end | ||
| )julia"; | ||
| std::vector<char> buf ( | ||
| /* no need to account for null since %s takes 4 bytes accounted for | ||
| * which is double counted by BUILD_RESOURCE_DIR and RUN_RESOURCE_DIR */ | ||
| strlen(julia_lib_load_template) + | ||
| strlen(BUILD_RESOURCE_DIR) + | ||
| strlen(RUN_RESOURCE_DIR), | ||
| /*c strings are null terminated*/ | ||
| '\0' | ||
| ); | ||
| snprintf(buf.data(), buf.size(), julia_lib_load_template, BUILD_RESOURCE_DIR, RUN_RESOURCE_DIR); | ||
| jl_eval_string(buf.data()); |
There was a problem hiding this comment.
While std::format would be nice, I feel like assembling this with std::stringstream and then calling jl_eval_string(stringstream_var.str().c_str() would've been a lot easier, but fair play to you for doing it C-style (no reason not to do it this way, it just sounds needlessly hard)
There was a problem hiding this comment.
Also how come std::format is the single feature where clang is further than gcc, very odd, usually gcc is much faster at implementing the new stuff.
|
|
||
| #include <.src/c_adapter.hpp> | ||
| #include "c_adapter.hpp" |
There was a problem hiding this comment.
Maybe I'm misunderstanding the purpose of replacing <> with "" everywhere, but the reason I intentionally kept the .src/ and include/ prefix was, so that when a user types in their IDE something like:
#include <
then I don't want my .src files showing up in the recommended completions (the thingy where when you press tab it auto-completes the statement). This is especially true with c_adapter, I don't want any of the users to even be aware what functions are in there, as it can break things in very odd ways. All files in .src and the entire c_adapter should be as hidden from end-users as possible.
Idk if this is true, but it feels like your restructuring made it, so users would have an easier time accessing the source and inline files, which I don't like. Correct me if I am wrong, though, as I'm not super sure what the full reason for the switch to "" was
There was a problem hiding this comment.
In gcc, clang, (and I think msvc) " based brackets search the current directory of the current file before searching the global include path. Where as < doesn't search locally first.
I moved them because they have to be installed some where to build a downstream library based on libjluna. When using a package manager, you can't assume that you have the cloned repo available, only the files explicitly installed.
Unfortunately, even with the < style includes, I still get auto complete suggestions for things in .src it's kind of unavoidable. There might be some other way to hide them, but it doesn't for me.
In the case where I don't want user's touching things, I typically use the PIMPL idiom to hide the implemenation and leave the details in the shared object as much as possible. Unfortunately that can be difficult with highly templatized code like this. I can't wait for wide support for modules when this can be a thing of the past.
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" The exact directory that everything is installed into is user specifiable using
The default is currently release if nothing else is set. What this does is if the user requests it via setting
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?
That certainly wasn't intentional if there were any problems. I think I missed merging one commit you made this afternoon. Cmake functions like |
Clemapfel
added
acknowledged
|
moved cmake update and windows support to #13 |
Clemapfel
removed
the
will_be_accepted
@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.
modified #include directives so that they work outside of installed as
a subtree of the project
modified how jluna.jl is found so that they
can be found if they are installed or just simply built, by default,
it prefers the local version from the installed versions. I would have
loved to use c++ std::format here rather than snprintf, but since the
supported compilers don't support it I can't use it here.
modified how libjluna_c_adapter.so is found to use the dynamic linker
instead of explicit path passing which improves relocatablity
rather than always forcing a Release build, allow the user to set it
to each of the CMake defaults
include CTest to make compiling the unit tests optional in the
standard way
include GNUInstallDirs which was not provided before
use find_library() to locate libjulia.so to make it more robust to
different OS layouts
use find_path() to locate julia.h to make it more robust to different
OS layouts
set the target_include_directories and target_link_libraries correctly
so that downstream consumers can simply
and everything works
simplified the logic to set the library name. Note that using
globalin the module is safe becauseglobals in julia are local tothe module they are defined in