Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

fingolfin
Copy link
Member

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:

  1. Which version to use for this JLL? It really shouldn't be the same as right now, as the list of dependencies changes, and that requires a new version.
  2. Which Julia versions should this JLL be made compatible with? Right now, this is built for Julia 1.5.3 or later, and thus only usable in Julia >=1.5. But we could make versions supporting 1.4 or even 1.3 if desired. But that's extra work and complexity, so I first would like to get this version working, and also hear explicitly that Julia 1.3/1.4 support is wanted, before spending time on it.
  3. The two first points are actually somewhat related: in order to support multiple Julia versions, the trick we used in other JLLs is to use the patch level of the version string to indicate the Julia version; that would suggest to reversion this package to something like 0.3.X where X is equal to the one in the Julia version 1.X. However, then packages using this JLL may need to be updated.
  4. I noticed that upstream has one newer commit than what is used here; rgcv/libcgal-julia@91382f4 -- so perhaps there should be an update using that? Could be done later, though

-DCMAKE_FIND_ROOT_PATH=${prefix} \
-DCMAKE_INSTALL_PREFIX=${prefix} \
-DCMAKE_TOOLCHAIN_FILE=$CMAKE_TARGET_TOOLCHAIN \
-DJulia_PREFIX=${prefix} \
Copy link
Member Author

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.

@fingolfin
Copy link
Member Author

Hmm, the two failed jobs died during Downloading artifact: CGAL -- hopefully a transient error? @giordano could you perhaps restart those two jobs?

@rgcv
Copy link
Contributor

rgcv commented Dec 30, 2020

@fingolfin you are too kind, thanks for pushing onward with all of these packages. Once again, amazing work, very much appreciated!

Which version to use for this JLL? It really shouldn't be the same as right now, as the list of dependencies changes, and that requires a new version.

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.

Which Julia versions should this JLL be made compatible with? Right now, this is built for Julia 1.5.3 or later, and thus only usable in Julia >=1.5. But we could make versions supporting 1.4 or even 1.3 if desired. But that's extra work and complexity, so I first would like to get this version working, and also hear explicitly that Julia 1.3/1.4 support is wanted, before spending time on it.

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!

The two first points are actually somewhat related: in order to support multiple Julia versions, the trick we used in other JLLs is to use the patch level of the version string to indicate the Julia version; that would suggest to reversion this package to something like 0.3.X where X is equal to the one in the Julia version 1.X. However, then packages using this JLL may need to be updated.

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.

I noticed that upstream has one newer commit than what is used here; rgcv/libcgal-julia@91382f4 -- so perhaps there should be an update using that? Could be done later, though

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:

  • Considering updating CGAL first (all missing versions if anyone else is interested, but in doubt, just the latest);
  • Bump the jll minor version and adopt the patch version to match julia's minor versions, similar to other jlls following this approach
  • Include the various patch versions to have the package be compatible with julia versions 1.3 through 1.5

Ping me if I missed something! :)

@rgcv rgcv mentioned this pull request Dec 30, 2020
@rgcv
Copy link
Contributor

rgcv commented Dec 30, 2020

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.

@fingolfin
Copy link
Member Author

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 Dependency("CGAL_jll"). The reason being that otherwise people can end up using an older CGAL_jll with a new libcgal_julia, or the other way around; and if you are not completely sure that this works (i.e. 100% binary compatibility), it can lead to crashes or worse (i.e., data corruption) and can be very annoying to debug.

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.

@rgcv
Copy link
Contributor

rgcv commented Dec 31, 2020

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.

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.

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 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.

I would also encourage you to consider adding a version bound to Dependency("CGAL_jll"). The reason being that otherwise people can end up using an older CGAL_jll with a new libcgal_julia, or the other way around; and if you are not completely sure that this works (i.e. 100% binary compatibility), it can lead to crashes or worse (i.e., data corruption) and can be very annoying to debug.

Solid advice! Would just v"5.2" suffice? Or do I have to resort to VersionSpec akin to libsingular_julia? Using the former, that would make libcgal-julia strictly compatible with v5.2.0 only (plus potential rebuilds if that is the case), hence why I assume it should be enough, correct?

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.

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.

@rgcv
Copy link
Contributor

rgcv commented Dec 31, 2020

Hmm thinking about it, VersionSpec using a MAJOR.MINOR version would probably be the way to go for depending on CGAL_jll since patches usually don't include any ABI nor API breaking changes that I'm aware of. In fact, minors don't even seem to break ABI compatibility either, but are likely to break some APIs, or just include newer features.

EDIT: I'm brewing something here I would like to try, pushing changes in the meantime.
EDIT2: Right, silly of me, can't push to your fork. Did not want to create yet another PR for this though. But hey, bit easier, I suppose.

@fingolfin
Copy link
Member Author

Closing this in favor of PR #2333

@fingolfin fingolfin closed this Jan 1, 2021
@fingolfin fingolfin deleted the mh/libcgal_julia branch January 22, 2021 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants