-
Notifications
You must be signed in to change notification settings - Fork 560
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
Update libcgal_julia to new libcxxwrap / libjulia #2326
Conversation
d4a39a8
to
32ca8ac
Compare
-DCMAKE_FIND_ROOT_PATH=${prefix} \ | ||
-DCMAKE_INSTALL_PREFIX=${prefix} \ | ||
-DCMAKE_TOOLCHAIN_FILE=$CMAKE_TARGET_TOOLCHAIN \ | ||
-DJulia_PREFIX=${prefix} \ |
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 took the liberty of sorting these arguments alphabetically and generally aligning them with other JLLs, in the hope that this will help a bit with future maintenance.
Hmm, the two failed jobs died during |
@fingolfin you are too kind, thanks for pushing onward with all of these packages. Once again, amazing work, very much appreciated!
CGAL itself has seen a few updates recently, having been released a couple of minor versions. Maybe I could look into updating CGAL's binaries first and then proceed with this. I'm even thinking of skipping straight to the latest version, unless someone might be interested in using other versions. In that case, just create as many PRs as there are missing versions.
I'd love to have compat from 1.3 up to nightly, at the very least until 1.6 is flushed out. I have seen some of the newer buildscripts, so I think I can handle that too. Unless you really really want to do that as well. You really don't have to, I can give it a shot!
I'm aware of that trick. I was thinking just bumping the minor version altogether, using then the patch version just like the other JLL's are doing. The only package using this JLL would be CGAL.jl, which I also published and maintained for a bit there (should get back into that too). Bumping the JLL's minor version would probably be best since it could be accompanied by the respective CGAL update, leading to a minor package update as well. Thinking that would just remove unnecessary hoops to jump through and everyone would be on the latest version.
This commit doesn't have much going on, if inspected. I just thought I should break some stuff apart and be more explicit about the version of JlCxx libcgal-julia depends on. Nothing terribly breaking, and I'm already omitting the patch portion of JlCxx's version, which should be fine. So in short:
Ping me if I missed something! :) |
xref #2331 <-- after that goes through, one could consider rebuilding libcgal-julia against this new version. To that end, I must update a few things on my end as well. Will take care of that ASAP. |
By all means, feel free to take this over; you can open your own PR and copy some or all the changes in this one, and we can close it. If you expect somewhat regular updates to CGAL and/or libcgal_julia in the future, it may make sense to copy the approach taken by e.g. libsingular_julia, which maintains the versions of the JLL for Julia 1.3/1.4/1.5 in parallel; if one follows that approach, three PRs are needed for an update: first for 1.3; then once that is merged (and ideally landed in the registry), one for Julia 1.4; finally the same for 1.5+. I would also encourage you to consider adding a version bound to Unfortunately due to a limitation in BinaryBuilder, this is currently limited to specifying an exact version match (see JuliaPackaging/BinaryBuilderBase.jl#93 for my attempt to fix this). That means that whenever you update CGAL_jll, you need to update libcgal_julia_jll in lockstep. Of course if there is an API/ABI breakage, that's necessary anyway; but it's annoying for minor bugfix updates... So I just hope my BBB PR (or some other fix) can be merged soon. |
Not necessary, I think we can leverage this PR already to avoid more pollution. I already did that with CGAL for reasons that transcend me.
I don't expect too frequent updates to CGAL/libcgal_julia right now, but for the time being, might as well take the extra step of providing compatibility starting with the latest LTS. Once 1.6 is out, I might just consider supporting it from then on instead. Lather, rinse, repeat.
Solid advice! Would just
I've been updating both in lockstep as it is, so it won't incur much of a change to the current workflow. Nonetheless, that would be a much wanted feature for plenty of maintainers I'm sure. Maybe even me in the future, I'm suspecting. |
Hmm thinking about it, EDIT: I'm brewing something here I would like to try, pushing changes in the meantime. |
Closing this in favor of PR #2333 |
Make some progress towards issue #2160
Should not be merged before @rgcv had a chance to review and comment. Also, there are some questions to discuss:
0.3.X
whereX
is equal to the one in the Julia version 1.X. However, then packages using this JLL may need to be updated.