search

JuliaGraphics / jlqml

C++ component of the QML.jl package
Other
9 stars 4 forks source link

Split makie support into a separate JlCxx module so it can be wrapped by a separate package #23

Closed steffenhaug closed 1 month ago

steffenhaug commented 1 month ago

This is probably not quite ready for a merge, but I'm starting a PR so we have a place to discuss where to go from here, and for visibility in case someone wants to play around with it or help out.

This PR supports https://github.com/JuliaGraphics/QML.jl/pull/207.

I opted to keep the C++ code backing QMLMakie in here for now. In the long term it might be interesting to separate it into its own JLL if it becomes huge, but I couldn't quite figure out the logistics of making a new JLL that links to this one so the Makie component can extend the generic OpenGL component.

barche commented 1 month ago

Thanks, I fully agree to keep the C++ code in one place, it doesn't add any special dependencies here and keeping two C++ JLLs in sync would be difficult. I also don't expect we will need to add much here, I prefer to implement as much as possible on the Julia side.

steffenhaug commented 1 month ago

I noticed CI failed on macOS. Is that normal?

I didn't think much more about it, seeing as you got it working, but now that I'm trying to move the application I'm developing at work over to use QMLMakie, I'm having some trouble compiling jlqml on macOS myself.

Or rather, I can compile it just fine at first glance, but when i then import jlqml_jll, I get an error at runtime about StdFill not being defined. I see that StdFill is only conditionally defined if JLCXX_HAS_RANGES, so I'm assuming that this isn't the case in whatever setup Yggdrasil is building libjlcxx_jll for macOS.

If I modify the cmake flags with sed as you have done to build jlqml in Yggdrasil, compilation fails because std::ranges, which is used in the headers for JlCxx, is not defined, which i suppose means that the checks in JlCxx to determine JLCXX_HAS_RANGES fails to catch that Apple Clang 15.0 with gnu++17 in fact doesn't have ranges.

If I manually #undef JLCXX_HAS_RANGES before I #include "jlcxx/stl.hpp" it actually does compile, but I can't see this in the Yggdrasil build, so I'm assuming its not normal to do that, and I wouldn't be surprised if it has some adverse effects.

I think the reason this ultimately happens, is that __has_include (<ranges>) succeeds, but ranges is defined in the namespace q20. Maybe that's an issue for libjlcxxwrap.

barche commented 1 month ago

Ah, yes, there have been many additions to libcxxwrap during this year's Google Summer of Code. I think you need to build libcxxwrap from source for this to work, but we triggered a bug in clang and it literally takes hours to compile now, so this is cumbersome. I have tested your code only on my Linux machine, but there is no inherent reason why it should cause problems, so any issues on Mac are undoubdetly related to libcxxwrap changes.

steffenhaug commented 1 month ago

I tried, and I noticed that. So is it effectively impossible to build on macOS for the time being? I guess I can leave it compiling overnight :-p