-
-
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
Windows: add wrapper .exe
to work around lack of RPATH
#35629
Conversation
98437c0
to
f383e78
Compare
Note that I'm carrying another commit in here that is part of another PR; that will be rebased out once #35626 is merged, this implementation uses some of that scaffolding. EDIT It was merged, this no longer carries any other commits. |
991a386
to
e62f006
Compare
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.
I guess the main concern here is that adding anything to PATH will break spawning cmd
for some people due to https://software.intel.com/en-us/articles/limitation-to-the-length-of-the-system-path-variable. (And perhaps that it'll take slightly longer to launch Julia and annoy people who are looking in task manager.)
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.
From CI, seems like we also need to reconfigure all the other dependency objects to move to the libexec folder also?
Seems somewhat tedious to have two "julia.exe" named things also (esp. since we expose unsafe_string(Base.JLOptions().julia_bin)
). Hm :/
No, I was just using a
Yes, it's slightly annoying. Luckily, however, as long as |
26dc2c9
to
d4e371d
Compare
d4e371d
to
2c7942e
Compare
@vtjnash is this good to merge? |
contrib/windows/exe_path_wrapper.c
Outdated
if (MAX_PATH_LEN < totalPathLen) { | ||
fprintf(stderr, "ERROR: Cannot append entries to PATH: not enough space in environment block. Reduce size of PATH!"); |
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.
I'm just concerned that it may not be unusual for some user paths to be already near this limit due to the system configuration (that the user might not be able to alter)
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.
I don't have any stats on this so you may be right, but I don't know what we can do about it; I haven't been able to come up with any valid alternatives.
Is the motivation for this that we want to add some directory that contains a Note that #36153 would remove the need to put anything on the |
2c7942e
to
3f866f1
Compare
Yes, specifically, the JLL stdlib effort. See #33973 and #35193 Using an MSIX won't solve this for all users; we need a solution that doesn't depend on how you install Julia and that works everywhere. |
I've altered this PR to not append to |
3f866f1
to
f7f8fb5
Compare
If I understand this correctly, then this whole If that is so, then I have to admit this seems unnecessarily complicated :) Can't you just put Or is this code also adding other folders to |
Not just I investigated using a wrapper |
f7f8fb5
to
23d82c5
Compare
What happens when you use In particular, I wonder if this hack in PyJulia would stop working: |
Right now, nothing changes since everything is still in In a dynamic language like Python, this is not a problem; you just search for and In a static language, when linking directly against |
Thanks for the clarifications.
Are the DLLs in one place? I thought the plan was to move them to individual artifact directories but my worry was that it's hard to find them. Or will they be listed in |
Eventually; no, they will not be in one place. From an already-running Julia REPL session, you'll be able to get their locations through things like You will be able to get it from the build system via things like |
For PyJulia, it'd be nice if I can just call (say) |
This Might using isolated applications and side-by-side assemblies be an alternative way to achieve something similar? I think I also still don't understand fully what the final directory architecture goal here is, i.e. which files are meant to go into which folder in the final design.
Well, the whole idea is that you put the |
23d82c5
to
fdbbfcd
Compare
Correct. So that means libLLVM, libgcc, etc... libjulia won't be moved, but everything else will.
No, unfortunately, they will not work. Native Assemblies do not have the ability to store a path to them within themselves, so you still need to add things onto
The end goal is for the Julia build system to use JLL packages for its binary dependencies. So everything having to do with
The problem with this is the number of tools that are already hardcoded to look for julia in |
Windows has no concept of `RPATH`, which makes shipping binaries with relatively-located dependencies quite challenging. Despite investigating many potential avenues of `RPATH` emulation, (including but not limited to Application Manifests, PE file patching, and bundling the true Julia executable as a resource inside of a launcher) the most reliable (And least breaking for external workflows) was found to be to create a launcher executable that invokes the true Julia executable from within the `libexec` directory. In order to not run afoul of PATH length limits, we store the original path in a separate sidechannel environment variable (`JULIA_ORIGINAL_PATH`) then restore it after startup is complete.
af73805
to
a0d2a92
Compare
This is passing all tests now, so it's ready to merge on my end. The only downside of this now is that we have to alter two tests:
These are acceptable downsides in my eyes; if anyone disagrees speak up now, otherwise I'll merge this sometime this weekend. |
I have not been following along here, but is it possible to do the setup and then exec the true Julia executable in the same process instead of starting it as a grandchild? That way it would have the expected relationship (child) and pid? |
Yeah, I agree those are a problem, and I liked where this was headed now. But I think we can fix those too. We just may need to make this wrapper extremely lightweight(*), use LoadLibrary (instead of spawning a new whole process) and then bootstrap up from there (using the PATH tricks here, but then there's also no length limit). (*) For example, see https://github.com/vtjnash/libwhich/blob/be900cd7ba35dd8357f08766b4f778d7243b5982/Makefile#L8-L14 and https://github.com/vtjnash/libwhich/blob/87cffe10080c98e7b5786c5166e420bf1ada1d41/libwhich.c#L100 |
I've looked for an
I'm not sure what you mean by this. Which libraries would I be loading? |
All of them. The links I sent show how you can make a binary that doesn't link against anything, then you can pull in each library as you need it for the next one. Not quite as good as |
I think what you're saying is to change the main If so, that will break a lot of code because |
I was pretty sure it couldn't do that before either, since the OS has no concept of a default or NULL handle (we shim it internally to search our list of standard places) |
Correct. The default behavior is for the compiler and kernel to cooperate on injecting code at start that calls LoadLibrary. We can intercept that by not populating its list of files, running our code, then calling LoadLibrary on the list just like it did. |
I'm trying to fully understand the folder architecture in this PR: Does In other words, we can thus safely add to path |
|
||
int wmain(int argc, wchar_t *argv[], wchar_t *envp[]) { | ||
// Determine absolute path to true julia.exe sitting in `libexec/` | ||
WCHAR currFileDir[MAX_PATH_LEN]; |
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.
Should be MAX_PATH
and same for the variable below if using PathCombine
A pointer to a buffer that, when this function returns successfully, receives the combined path string. You must set the size of this buffer to MAX_PATH to ensure that it is large enough to hold the returned string.
https://docs.microsoft.com/en-us/windows/win32/api/shlwapi/nf-shlwapi-pathcombinew
exit(1); | ||
} | ||
PathRemoveFileSpec(currFileDir); | ||
PathCombine(juliaPath, currFileDir, TEXT(JULIA_EXE_PATH)); |
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.
PathCombine is not recommended from Microsoft:
Note This function, PathCchCombineEx, or PathAllocCombine should be used in place of PathCombine to prevent the possibility of a buffer overrun.
https://docs.microsoft.com/en-us/windows/win32/api/shlwapi/nf-shlwapi-pathcombinew
// within the callee. | ||
LPWSTR pathVal = (LPWSTR) malloc(MAX_PATH_LEN*sizeof(WCHAR)); | ||
DWORD dwRet = GetEnvironmentVariable(TEXT("PATH"), pathVal, MAX_PATH_LEN); | ||
if (dwRet > 0 && dwRet < MAX_PATH_LEN) { |
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.
I think this should be !dwRet && dwRet <= MAX_PATH_LEN
However looking at the documentation for GetEnvironmentVariable
. we should handle resizing the buffer instead of assuming MAX_PATH_LEN
After reading this I'm still unclear on the implications and I certainly can't interpret the PR itself, so I had better check this specifically. I have an application that effectively uses Julia as a plugin module. On Windows it relies on the fact that having the Julia bin directory in PATH implies that
and that's all the loading that is necessary. Will this change in any way with this PR? |
I have to say this
Wouldn't another option be to just ship My sense from reading a fair bit on this is that at the end of the day the platform wants us to put load time dynamically linked DLLs into the same folder as the |
closing in favor of #36588 |
Windows has no concept of
RPATH
, which makes shipping binaries withrelatively-located dependencies quite challenging. Despite
investigating many potential avenues of
RPATH
emulation, (includingbut not limited to Application Manifests, PE file patching, and bundling
the true Julia executable as a resource inside of a launcher) the most
reliable (And least breaking for external workflows) was found to be to
create a launcher executable that invokes the true Julia executable from
within the
libexec
directory.