-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Load libpcre2-8 with Libdl to solve windows embedding issues
#60677
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
base: master
Are you sure you want to change the base?
Conversation
libpcre2-8 with Libdl to solve windows embedding issues
|
Thanks again for taking up this initiative.
Was testing exhaustive or random? Do you have a test script that I can run as well as part of my review? Is it possible to do the |
base/libc.jl
Outdated
| throw(ArgumentError("invalid arguments")) | ||
| end | ||
| @static if Sys.isapple() | ||
| function callmktime(s::String) |
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 function doesn't call mktime, so it could probably benefit from a rename.
Exhaustive, so it did not feel sensible to add it to @testset begin
nerrors = 0
for N = 1:8
for chrs in Iterators.product(fill(['a','/', '.', ':', '😺', '\n'], N)...)
nerrors > 20 && break
str = String(collect(chrs))
if splitpath(str) == UUT.Filesystem.splitpath(str)
@test true
else
nerrors += 1
@show str
@show splitpath(str)
@show UUT.Filesystem.splitpath(str)
end
end
end
end
Makes sense. |
Agreed - still valuable testing but not needed in CI
Sure thing, happy to donate the machine time to test this on unix. |
|
By the way, what would be the scope of versions to support? Since the embedding issues with |
This reverts most commit 3bd137c. Most changes to path.jl and file.jl are not reverted.
|
Here are the tests I used while porting pathjl_porting_tests.zip |
Purpose
The current situation is that
Base.PCREusesccall((sym, "libpcre2-8"),...). On windows, this only works fine when using Julia.exe or on systems wherelibpcre2-8is available on PATH (such as when having julia installed). Embedding and JuliaC dlls on windows however does not detect the bundledlibpcre2-8. I have reported this issue w.r.t. JuliaC in #60496, #60406 and JuliaLang/JuliaC.jl#83 while understanding the root cause better. #52007, #52205, MilesCranmer/PySR#566 and MilesCranmer/PySR#636 are likely caused by the same core issue.To solve this, the goal is to use
BundledLazyLibraryPathinBase.PCRE. However, the latter is used to for parsing regular expressions, which is necessary for building the loading path used inLibdl. So currentlyBase.PCREmust be able to loadlibpcre2-8beforeBundledLazyLibraryPathmay be used. This catch 22 is discussed in #60496 and it was concluded that revisingpath.jlwas the best way forward.This pull request contains a rework of
path.jlto avoid regular expressions, so that all path operations are available beforelibdl.jlis loaded. With this in place, some additional reorganization of the loading order inBase.jlare provided. I'm open for proposals on better ways to organize these changes, but current state is sufficient for solving the main problem: Windows dlls built with JuliaC now works on systems without the julia bin-folder on PATH.Description of Changes
Loading order in
Base.jl* `libc.jl` loaded before `regex.jl`. * Add `path.jl` earlier, instead of being included in `filesystem.jl`. `module Filesystem` is now defined in `path.jl` and the rest is added in `filesystem.jl`. In the end, the content of `Filesystem` should be unaffected. * Similarly, extract `iswindows`, `isunix` etc. into a new file `osinfo.jl` which also defines `module Sys` and is loaded early. These operations are used frequently in `path.jl` and `libdl.jl`. The rest of `Sys` is added later. * Some minor changes to other files.Update: This PR will only cover the updated path code.Path.jl modifications
All regular expressions in
path.jlare replaced with corresponding string operations, with the goal of not altering any behavior whatsoever. The differences between windows/unix is now captured by the functionisseparator(and different joinpath implementations, which I did not modify).I would like to stress that I have (NB: on windows) tested all algorithms exhaustively using sufficiently long strings comprising applicable chars like
('a', '\alpha', ':', '.', '/', '\n')to ensure consistency with the regex implementations. It was indeed tempting to fix some obvious bugs, but I only added test cases to capture that this is the current behavior and commenting that the behavior is questionable.Performance should not be degraded. With simple tests like
@btime joinpath("foo", "bar"), performance is in fact improved by at least a factor of 2 everywhere. As extreme examples,splitdrive("foo/bar")goes from 250 to 12 ns andisabspath("foo/bar")from 100 to 8 ns (also just tested on windows).To do
Trimming
I have verified that this PR so far solves the issue when using JuliaC with#60496 (comment).trim = "no". But I cannot find a way to makedlopen(PCRE_LIB)precompile when trimming; I constantly run into unresolved calls. Ifdlopenis not called in precompilation, there are runtime errors instead. I do not understand the mechanisms involved in making a type-instableLazyLibrarycompile statically well enough to troubleshoot this further. Can I ask for some help here?Other changes
As already mentioned, I am very open for proposals on how to improve this. The code has already served its purpose by proving that these changes were sufficient for fixing the JuliaC issue.
Should we use only some of the changed code in
path.jl? Is there some better way to handle file structure & loading order changes inBase.jl?