-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add load_libjulia in libjulialoader #37779
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API is still fundamentally broken. It should not be required for the user to pass in the path to the julia executable since that's extremely error-prone and may be impossible.
Ref #36588 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly how I would have done it.
I noticed that calling In [1]: import ctypes
...: libjulialoader = ctypes.CDLL("usr/lib/libjulialoader.so")
...: libjulialoader.load_libjulia.restype = ctypes.c_void_p
...: libjulialoader.load_libjulia(b"usr/bin")
Out[1]: 94264976507248
In [2]: libjulialoader.load_libjulia(b"usr/bin")
Out[2]: 94264976504784
In [3]: libjulialoader.load_libjulia(b"usr/bin")
Out[3]: 94264976504784
In [4]: libjulialoader.load_libjulia(b"usr/bin")
Out[4]: 94264976504784
In [5]: libjulia = ctypes.PyDLL("usr/lib/libjulia.so", ctypes.RTLD_GLOBAL)
In [6]: libjulia._handle # same as Out[1]
Out[6]: 94264976507248 I'm testing this on Linux. I thought |
Ah, I guess this is because Lines 146 to 147 in da2935d
mutates the global Edit: Ye, It looks like it. With this patch diff --git a/cli/loader_lib.c b/cli/loader_lib.c
index ed989481b6..1b291bfb6c 100644
--- a/cli/loader_lib.c
+++ b/cli/loader_lib.c
@@ -40,6 +40,7 @@ static void * load_library(const char * rel_path, const char * src_dir) {
strncat(path, src_dir, sizeof(path) - 1);
strncat(path, PATHSEPSTRING, sizeof(path) - 1);
strncat(path, rel_path, sizeof(path) - 1);
+ print_stderr3("`dlopen`ing: ", path, "\n");
void * handle = NULL;
#if defined(_OS_WINDOWS_) I get In [1]: import ctypes
...: libjulialoader = ctypes.CDLL("usr/lib/libjulialoader.so")
In [2]: libjulialoader.load_libjulia(b"usr/bin")
`dlopen`ing: usr/bin/../lib/libgcc_s.so.1
`dlopen`ing: usr/bin/../lib/libopenlibm.so.3
`dlopen`ing: usr/bin/../lib/libjulia.so.1.6
Out[2]: -1558041792
In [3]: libjulialoader.load_libjulia(b"usr/bin")
`dlopen`ing: usr/bin/../lib/libgcc_s.so.1
Out[3]: -1558044256
In [4]: libjulialoader.load_libjulia(b"usr/bin")
`dlopen`ing: usr/bin/../lib/libgcc_s.so.1
Out[4]: -1558044256
In [5]: libjulialoader.load_libjulia(b"usr/bin")
`dlopen`ing: usr/bin/../lib/libgcc_s.so.1
Out[5]: -1558044256 |
And you have still not answered the question of how are the user supposed to find the binary reliably. And the motivation for making embedding much harder than necessary. |
@yuyichao Yes, you've made your concerns clear, but realize that the work done here was arrived at after much analysis (and even a couple failed PRs). It was just getting too awkward for libLLVM to be loaded one way, while all our other dependent libraries get lazy loaded if possible. While we realize some of this work is still incomplete, the majority of the foundational work is being finished with this PR to permit making it easier to link against libjulia, while yet enhancing our ability to select, load, and upgrade the dependent libraries in situ. Merging that PR was necessary to help unstick some other pending work, so we opted to merge it early to give an opportunity that all of the pieces get testing and fixes now (thus further in advance of the actual release branch date) instead of being blocked on getting testing of all pieces on all platforms. |
Where are they? As I already mentioned, the one failed PR about adding an executable wrapper on windows does not seem to have any applicable objection on it. (All objection are related to the change of
Not sure how this is related. Here I'm not even talking about lazy vs not. Everything done here are eagerly loaded (and for libjulia dependency that's totally fine) and it was eager as well so nothing was changed. AFAICT the libraries that currently get lazily loades will still remain to do so, so nothing should have changed about the lasiness either. There are and will always be two different ways to load thing, one that must happen before or at the same time
Exactly, the implementation has many other problems that I was not even commenting much on. Having other stuff pending on this does not mean the change is good to go. The implementation can be bad but the design has to be sound. Most of what I was focusing on, and the only point I mentioned here, are AFAICT fundamental problems tied to how this is designed, the API, and not about the implementation. Here, I'm only asking about the public API change for embedding. Unless future progress will completely remove the |
Let's focus in on |
Well, anything that does not require the user to specify additional path on the API, i.e. equivalent to specifying Also I don't see why libraries linked to
is a design constraint) |
One of the fundamental design patterns I want to avoid is platform differences. We build these platform abstraction libraries precisely so that users don't have to worry about what platform they're running on. In the end, I don't see requiring passing in an anchoring path as a large burden, and since I don't see a feasible way around it that will still allow us to give the same guarantees of loading precisely the libraries that we want to, it seems like the best solution still.
This is best summarized in this comment. The benefits to this system are:
|
Yes, that is a good goal, however,
I did give concrete arguments about this. Since that's what you ask for, please address how each usecase can be solved specifically rather than just say you don't see something as a large burden because it isn't a problem for the julia executable. As I said, the binary may or may not exist and the user may or may not be able to find the correct one. One more thing is that code like
are virtually impossible to port without a very major rewrite where the user has to match how libjulia is loaded, leaking the abstraction. The symbols are not going to be available for the compile time linker anymore. And since abstracting out the platform is only necessary for the user, using RPATH when it exists is a perfectly good solution.
Whatever unpack them should be able to unpack them into either just fine.
The same, the JLL package, especially since the library path doesn't have to be constant anymore, does not have to point to the In another word, these are only "benefit" because you want to put the special logic at a particular place. They are in reality not benefit at all when that logic can move. None of these avoid the logic anyway. More over, the artifact path can even be itself encoded in the RPATH and you don't even have to move the file. |
is this still needed? IIUC, we have done something similar now |
Yes, it looks like #38160 handles this more automatically. |
From the discussion in #36588 (comment), I'd imagine we need something like
void * load_libjulia(const char *)
for loadinglibjulia
in a cross-platform manner.This is a dead-simple implementation that just extracts out the first lines of
load_repl
. @staticfloat If you have a better idea of doing this, please feel free to close this and implement it in a new PR.Here is a simple Python session with this PR:
Question: Should it be renamed to
jl_load_libjulia
or something?cc @davidanthoff @GunnarFarneback