-
Notifications
You must be signed in to change notification settings - Fork 559
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
libcxxwrap_julia: switch to libjulia #1948
Conversation
d8dc551
to
87b33e0
Compare
This comment has been minimized.
This comment has been minimized.
To fix mingw please try adding And there is no libjulia_jll for freebsd (yet?): Yggdrasil/L/libjulia/common.jl Line 180 in fddff44
|
This comment has been minimized.
This comment has been minimized.
0028a7e
to
efa8489
Compare
This seems to work now! Thanks to @benlorenz for the help. @barche care to take a look? |
This looks great to me, thanks for doing this. Just two doubts:
|
Let's wait for PR #1951 and its two follow-ups, then I'll redo this PR . |
efa8489
to
a268858
Compare
Updated for that latest libjulia_jll. Unfortunately there is a compile error on FreeBSD: Regarding |
We could go without FreeBSD (as long as @ararslan doesn't see us), but the error is rather weird 🤔 |
9be6db1
to
85921a5
Compare
But I guess the compiler might not pick up the include path where |
With commit |
Probably because on FreeBSD (in general and not just in BB) Otherwise adding
|
Huh, OK, interesting. So how come other packages which rely on headers from JLLs installed as (build) dependency do compile on FreeBSD? Are they all already adding in explicit |
85921a5
to
57420fc
Compare
@barche I switched to that commit and it seems to work as intended |
In some cases cmake or configure might determine that these are needed, so this could probably be fixed in FindJulia as well. But it is also given in the documentation that it might be needed in some cases (a quick grep found about 100 such flags in the recipes): https://juliapackaging.github.io/BinaryBuilder.jl/dev/troubleshooting/#Header-files-of-the-dependencies-can't-be-found In libjulia there is also |
I hope it's not rude to push changes, but I think with the latest update of FindJulia it will work. Fingers crossed. |
Not rude at all, to the contrary; in fact, I was somewhat worried when I made this that you'd find me rude for doing a PR for "your" JLL... but I figured it'd be useful to get this kicked off to find obstacles and remove them, and, well, that's exactly what happened ;-). In fact, I am somewhat worried whether the JLL this PR produces works in practice -- so I think before this PR is merged, we really should test it locally on some systems (and even before that, perhaps download the tarballs created by the CI runners and inspect the binaries in them). |
I did a first test with the |
Since this seems to work well now, how shall we proceed? Merge this, then @barche proceeds to make Julia 1.4 and 1.5 variants? |
Yes, that's the idea, but this also reminds me: wasn't there a way to limit the Julia compat in the JLL? It would be better to have this done in the JLL rather than in CxxWrap.jl. In that case, the limit for this JLL should be Julia 1.3, but I forgot how this is done. |
Using But! there is a caveat: packages with the the same version, just different build version (the |
Perhaps one could abuse the prerelease part of the version string? I.e. use versions |
63045eb
to
7ba940d
Compare
I've rebased and squashed this. I've also added a Another thing: We could split this recipe now into multiple (like we did for e.g. libjulia), that coexist; that might make maintenance a tad easier in the future? (Or not, dunno) |
7ba940d
to
391604e
Compare
OK, all working well now :-). |
This looks good to me! |
L/libcxxwrap_julia/build_tarballs.jl
Outdated
name = "libcxxwrap_julia" | ||
version = v"0.8.0" | ||
version = VersionNumber("0.8.2-julia.$(julia_version.major).$(julia_version.minor)") |
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.
What's the rationale of this version number? Pkg doesn't really like anything outside of X.Y.Z and we already overstress it with the build number.
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.
Here's the reason: We need to build separate binaries (artifacts) of libcxxwrap for *at least Julia 1.3 versus Julia >= 1.4; but better, for each of Julia 1.3, 1.4, 1.5, 1.6. Each of the corresponding JLL wrappers needs a different compat
entry for Julia in Project.toml
. But to do that, they need actual different versions (the build number part after the +
is not enough for that, the registry doesn't support that, which makes sense, as it is consistent with how semver defines build number).
Unlike build numbers, the pre-release is part of semver which is explicitly defined to indicate different versions, including how they should be compared with each other and other versions. It also is used by Julia itself As such, I would hope that existing Julia tooling correctly deals with it (if not, that'd be a bug in the semver support of that tool, which IMHO should be fixed).
One drawback is that the semver spec says this "A pre-release version indicates that the version is unstable and might not satisfy the intended compatibility requirements as denoted by its associated normal version." However, that should not matter for any tooling, it's just something about the semantics for users; and the only user of this JLL would be CxxWrap.jl, so there should be no issue here, I think.
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 think this is OK, as long as it works. Not knowing about pre-releases, what I had in mind for this was:
- libcxxwrap 0.8.2 = Julia 1.3
- libcxxwrap 0.8.3 = Julia 1.4
- libcxxwrap 0.8.4 = Julia 1.5
- libcxxwrap 0.8.5 = Julia 1.6
Starting from Julia 1.6, I am hoping this is solved by the julia version being part of the artifact resolution, if I understood correctly?
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.
Starting from Julia 1.6, I am hoping this is solved by the julia version being part of the artifact resolution, if I understood correctly?
Well, that would work for Julia v1.6+
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.
@barche if you are willing to go through that hassle, sure, why not (but perhaps shift all the libcxxwrap version up by one, then the last digits of the libcxxwrap and Julia version match, that might make things easier?).
Of course it also raises the question how you then would version a bugfix release? Perhaps by adding 10 to the patchlevel each time?
- libcxxwrap 0.8.3, 0.8.13, 0.8.23, ... = Julia 1.3
- libcxxwrap 0.8.4, 0.8.14, 0.8.24, ... = Julia 1.4
- libcxxwrap 0.8.5, 0.8.15, 0.8.25, ... = Julia 1.5
- libcxxwrap 0.8.6, 0.8.16, 0.8.26, ... = Julia 1.6
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.
? A single version wouldn't allow to produce several binaries for several Julia versions, though? And hence wouldn't allow to resolve the crashes with Julia 1.3.
Also, the current release is 0.8.0, hence this should be at least 0.8.1.
It is also far from clear to me that this is a temporary situation which will go away with Julia 1.6 (plus, it will likely be months until then, won't it? : first off, that assumes everyone is ready to immediately drop support for all Julia versions once 1.6.0 is released. Secondly, it is not yet clear to me that Julia 1.6 will really resolve this: are all pieces already there? Even if, without verifying that it works as desired in practice, I'd be reluctant to rely on that (no offense intended to @staticfloat and anybody else who may habe worked on this).
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.
No, what I mean is keep the proposed mapping as:
- libcxxwrap 0.8.2 = Julia 1.3
- libcxxwrap 0.8.3 = Julia 1.4
- libcxxwrap 0.8.4 = Julia 1.5
- libcxxwrap 0.8.5 = Julia 1.6
If you think bugfixes are important for older Julia versions, we could do:
- libcxxwrap 0.8.2 = Julia 1.3
- libcxxwrap 0.9.0 = Julia 1.4
- libcxxwrap 0.10.0 = Julia 1.5
- libcxxwrap 0.11.0 = Julia 1.6
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.
As long as they are all using the same source, I'd suggest to keep them all at 0.8.x
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've changed the version (back) to 0.8.2, as discussed here. Can this be merged now, or is anything still to be done? (I am not aware of anything, but...)
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.
OK for me!
Also drop the restriction to the cxx11 ABI and add more platforms, and allow building multiple versions of this targeting different Julia versions. Co-authored-by: Bart Janssens <bart@bartjanssens.org> Co-authored-by: Benjamin Lorenz <benlorenz@users.noreply.github.com>
391604e
to
d65360d
Compare
Also drop the restriction to the cxx11 ABI and add more platforms.
Motivation: while this is not the perfect next step for libcxxwrap_julia, I think it makes sense to do this now: switching to libjulia and adding more platforms will allow other packages using libcxxwrap_julia to do the same. Once (if?) we get "full" support for "julia_version" as part of the platform, adjusting
libcxxwrap_julia
and other JLLs depending on it to benefit from those shouldn't be hard; but it is unclear when this will be available; so why not implement those benefits we can already get now?As a side effect, this will produce a fresh JLLWrapper based JLL :-)
However, this PR shouldn't be merged without @barche approving.
UPDATE: ... and of course it should pass the CI tests before all else ;-)
Close #1647