JuliaInterop / libcxxwrap-julia

C++ library for backing CxxWrap.jl
Other
84 stars 43 forks source link

julia 1.11: add some workarounds for jl_array changes #137

Closed benlorenz closed 1 year ago

benlorenz commented 1 year ago

for the changes in https://github.com/JuliaLang/julia/pull/51319

cc: @fingolfin

Edit: The library seems to build but a test module fails with:

[ 87%] Linking CXX executable test_module
cd /home/runner/work/libcxxwrap-julia/libcxxwrap-julia/build/test && /usr/local/bin/cmake -E cmake_link_script CMakeFiles/test_module.dir/link.txt --verbose=1
/usr/bin/c++  -Wunused-parameter -Wextra -Wreorder -fPIC -g CMakeFiles/test_module.dir/test_module.cpp.o -o test_module   -L/opt/hostedtoolcache/julia/nightly/x64/lib  -L/opt/hostedtoolcache/julia/nightly/x64/lib/julia  -Wl,-rpath,/opt/hostedtoolcache/julia/nightly/x64/lib:/opt/hostedtoolcache/julia/nightly/x64/lib/julia:/home/runner/work/libcxxwrap-julia/libcxxwrap-julia/build/lib ../lib/libcxxwrap_julia.so.0.11.1 /opt/hostedtoolcache/julia/nightly/x64/lib/libjulia.so.1.11 
/usr/bin/ld: CMakeFiles/test_module.dir/test_module.cpp.o: in function `jl_datatype_layout':
/opt/hostedtoolcache/julia/nightly/x64/include/julia/julia.h:1310: undefined reference to `jl_unwrap_unionall'

~Not yet sure what to make of this.~ This might be fixed by https://github.com/JuliaLang/julia/pull/51916.

PallHaraldsson commented 1 year ago

I'm just curious did Julia break the C API? I mean it's not guaranteed stable yet I believe, except possibly some subset. But still practical breakage? Or is this about an added feature (that you could choose to now support)?

benlorenz commented 1 year ago

I'm just curious did Julia break the C API? I mean it's not guaranteed stable yet I believe, except possibly some subset. But still practical breakage? Or is this about an added feature (that you could choose to now support)?

jl_arrayset seems gone (this was exported from libjulia.so) and jl_array_data needs a seconds (type-)argument now. These are the ones that I noticed for libcxxwrap. So yes I think this is a changed C API, and if I remember correctly it was mentioned that this is expected to happen from time to time.

fingolfin commented 1 year ago

Julia unfortunately has breaking C API changes all the time -- though at least these days they seem to avoid them in patch releases.

PallHaraldsson commented 1 year ago

It should at least be guaranteed to not happen in point releases, IMHO. Or people must then not upgrade (those who embed Julia, and well use C++ with...). There also doesn't seem like no reason for it (backporting needs to happen, but selectively, I doubt breaking the API needed for security, or even bug-fixes), Julia must be getting stable over time. So in fact also seems not needed for 1.x of changing x.

Adding to the API should be ok, I feel like at least a subset of the C API should be usable. Calling C code is ok, that has never been broken, so I'm unsure why does C++ actually need more, i.e. any of the C (embedding) API? Could this project restrict itself to some stable subset?

fingolfin commented 1 year ago

We are talking here about fixes for the julia development version, not backports.

basically, for C API changes, minor releases 1.x are treated as semver major releases (so breaks happen) and patch releases as minor releases . Not great but I can see why to a degree. They are quite explicit about promising compatibility only at the language level, not for internal kernel APIs.

And nothing here is specific to C++

barche commented 1 year ago

Regarding the jl_arrayset function I think it would be better to fall back to what is described in the embedding docs (link: https://docs.julialang.org/en/v1/manual/embedding/#Updating-fields-of-GC-managed-objects, I missed this when writing the array wrapper):

jl_array_t *some_array = ...; // e.g. a Vector{Any}
void **data = (void**)jl_array_data(some_array);
jl_value_t *some_value = ...;
data[0] = some_value;
jl_gc_wb(some_array, some_value);

I suppose that way we can also avoid the -ljulia-internal in the test?

benlorenz commented 1 year ago

Regarding the jl_arrayset function I think it would be better to fall back to what is described in the embedding docs (link: docs.julialang.org/en/v1/manual/embedding/#Updating-fields-of-GC-managed-objects, I missed this when writing the array wrapper):

jl_array_t *some_array = ...; // e.g. a Vector{Any}
void **data = (void**)jl_array_data(some_array);
jl_value_t *some_value = ...;
data[0] = some_value;
jl_gc_wb(some_array, some_value);

This would be similar to the data()[i] = x in push_back for ArrayRef? Except that I am a bit unsure about the element type of that for Array, but void** would probably work. I can have a look later today.

Right now the only failure in the tests seems to be https://github.com/JuliaInterop/CxxWrap.jl/pull/390 or am I missing something?

I suppose that way we can also avoid the -ljulia-internal in the test?

That workaround was only needed for avoiding the jl_unwrap_unionall issue (and worked only on linux), I removed the commit now.

PallHaraldsson commented 1 year ago

FYI, I just noticed with example "C++ Calling Julia", just about to be registered in case helpful to you: https://github.com/Suzhou-Tongyuan/TyJuliaCAPI.jl

TyJuliaCAPI.jl provides a stable C API for Julia. [..] We have [several] projects implemented in C++/C#, and calling Julia from native code is a crucial requirement. [..] We, therefore, believe that the current quality of Julia's C API is insufficient to support product development and maintenance.

And we need a stable and generic Julia C API.

In the past, we have learned a useful technique from PythonCall.jl (referred to as GC Pooling), which PythonCall itself used to provide a set of interoperation mechanisms, surpassing similar projects in terms of stability and performance. Following this technique, we have implemented a stable and well-designed C API for Julia, which we call TyJuliaCAPI.

In TyJuliaCAPI, the lifecycle management is adopted in a manner similar to the Python Stable C API (the most widely applied and highly regarded C API design)

fingolfin commented 1 year ago

Thanks for the pointer, but no, TyJuliaCAPI.jl is not helpful here. (and while I think it is a noble effort, some of the comments in the README strike me as rather naive -- holding up Python as a paragon for stable APIs, much less ABIs completely ignores history. The Python C API had over 30 years to evolve, and is notoriously is not ABI stable, and has other issues -- one of multiple reasons why e.g. HPy is a thing. We definitely want a more stable C API for Julia at some point, and there certainly are lessons to be learned from Python and other systems; we (OSCAR team) have suffered from and worked through Julia API/ABI changes constantly, so we appreciate the need. But at the same time there are good reasons why Julia is not yet there... In any case, any efforts to get such a stable API without involving the Julia core dev team seems doomed to fail ... though perhaps I just am just to old and cynical when it comes to this kind of stuff ...)

That said, this is all a red herring, it doesn't help us one bit with the issue at hand.

benlorenz commented 1 year ago

I think the code here should be working, the tests still fail in the CI due to https://github.com/JuliaInterop/CxxWrap.jl/pull/390, locally I get even more errors, see #391, because CxxWrap uses some internal string iteration methods that stopped working for the wide C++ strings. Those errors don't appear in CI because it seems to use the testjll branch of CxxWrap.jl which is slightly outdated.

With the changes from this PR, https://github.com/JuliaInterop/CxxWrap.jl/pull/390 applied and https://github.com/JuliaLang/julia/pull/51671 temporarily reverted I got all tests to pass locally.

benlorenz commented 1 year ago

I changed the code to use jl_array_ptr_1d_push instead which does exist on all supported julia versions and simplifies the code quite a bit, this uses the (soon) recommended jl_array_ptr_set internally which deals with the write barrier.

PallHaraldsson commented 1 year ago

Python has (limited) stable API/ABI and maybe Julia should define similar JL_LIMITED_API: https://docs.python.org/3/c-api/stable.html

There are two tiers of C API with different stability expectations: [..] When Py_LIMITED_API is defined, only this subset is exposed from Python.h. [..] To enable this, Python provides a Stable ABI: a set of symbols that will remain compatible across Python 3.x versions. The Stable ABI contains symbols exposed in the Limited API, but also other ones – for example, functions necessary to support older versions of the Limited API.

I've not looked closely at TyJuliaCAPI.jl how they can promise a stable C API (and ABI?!) for Julia, presumably on top of its unstable C API, probably by locating the most practically stable subset, or thinking least likely to change, that could be defined stable (but hasn't...); for JL_LIMITED_API. I'm guessing that package is more of a promise to fix in the future if Julia changes... to at least localize in one place needed changes?

barche commented 1 year ago

Tests here use the testjll branch of CxxWrap.jl, which I updated now to include the change from https://github.com/JuliaInterop/CxxWrap.jl/pull/390, but because of https://github.com/JuliaInterop/CxxWrap.jl/issues/391 nightly will probably still fail.