-
Notifications
You must be signed in to change notification settings - Fork 558
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
Improve libjulia, rebuild libjulia 1.3.1 with the improvements #1951
Improve libjulia, rebuild libjulia 1.3.1 with the improvements #1951
Conversation
OK so I made two Julia PRs to fix warnings when compiling for FreeBSD, see JuliaLang/julia#38139 (also fixes a genuine threading bug, I think) and JuliaLang/julia#38140. But unfortunately linking fails:
Googling that lead me to AcademySoftwareFoundation/OpenShadingLanguage#1069 which lead me to JuliaLang/julia#35364. And indeed, Julia 1.4 is missing Next up is 1.5; there we run into a download issue:
|
Ok now it works on 1.5, too 🎆 |
gotta still check that I broke nothing else... so I'll now enable a full build of all three versions in all platforms... for debugging, shouldn't be merged |
9707f2e
to
a1c5799
Compare
I've enabled more platforms now, and am seeing build issues that I already saw for the Julia 1.4.2 build, see #1704 (comment) -- so this used to work, and likely some of the changes that were made since then to libjulia_jll (mostly by me...) broke it. On the upside, perhaps that means we can get ARM 32bit builds to work across all Julia versions again. We'll see. |
7cab411
to
d49c599
Compare
While FreeBSD now works, I noticed that Julia 1.5 now also fails in ARM 32 builds, too, with the same error as discussed here. On the upside, that suggests that we might be able to fix this also for 1.3 and 1.4, too. The first suggestion here is to to add
This issue is also discussed in JuliaLang/julia#36371 so perhaps @yuyichao has some insights? I have a suspicion that one needs to set a different |
You can allow with |
D'oh, actually we already use |
Wooohooo that did it :-) |
478717d
to
1fb0434
Compare
Status:
Anyway, I gotta run now, so I'll only debug this later. If somebody knows the above issue and has a solution, please chime in. |
A fix (from JuliaLang/julia#36821) for the macOS 1.5 |
1fb0434
to
6b29018
Compare
And regarding libunwind, according to the readme it requires setcontext and getcontext which musl doesn't provide. For x86_64 libunwind has it's own implementation but not for i686, see libunwind/libunwind#69. So maybe the musl i686 build for LibUnwind should be disabled? |
9452343
to
ed007bf
Compare
@benlorenz thanks for the help, I got the macOS port fixed. And I won't fret about 32bit musl builds, I'll just keep them disabled. |
To proceed with this, I'll let all builds run one more time to see if I got it right. Then I will submit this in three staged PRs: the first updates libjulia 1.3; the next then 1.4; and finally 1.5. |
f95acae
to
8e1f25d
Compare
This PR now only rebuilds Julia 1.3, and is to be ready for review and merging. |
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.
This is really impressive! Quite obviously a work of much patience.
L/libjulia/common.jl
Outdated
@@ -120,7 +121,7 @@ function configure(version) | |||
# julia expects libuv-julia.a | |||
override LIBUV=${prefix}/lib/libuv.a | |||
|
|||
override BB_TRIPLET_LIBGFORTRAN_CXXABI=${bb_full_target} | |||
override BB_TRIPLET_LIBGFORTRAN_CXXABI=${bb_full_target%-julia_version*} |
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.
Practically, this is fine, but theoretically there's no guarantee on the ordering of tags in a platform triplet.
I suggest we extend the bb
command to allow the build script to ask for the platforms it wants. E.g. bb get-triplet --libgfortran --cxxabi
.
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.
(This is not a request on this PR, more a note that we should do this eventually)
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.
But isn't the precise value forbb_full_target
what is used to generate the file names of the binary archives we upload to GitHub? And the precise value of BB_TRIPLET_LIBGFORTRAN_CXXABI
(resp. the other BB_TRIPLET*
values derived from it) what is then used to generate the download URLs? So they need to match?
What am I missing?
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, the environment variable ${bb_full_target}
within the build environment has the full concrete specification of the target, including C++ string ABI and libgfortran version, even if those details aren't relevant for the generated tarballs
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'm saying that your bash
variable substitution here says "take $bb_full_target
and remove everything after -julia_version
", but if the triplet gets serialized as x86_64-linux-gnu-julia_version+1.6.0-cxx11-libgfortran3
, then this obviously won't work.
Practically speaking, that's never going to happen because due to historical reasons we'll always put libgfortran
and cxx11
first, but some day in the future we may have other tags that are relevant here and they may or may not be placed before/after the julia_version
tag. So I'm saying it would be good for you as the builder to ask BB for a triplet containing a certain subset of tags.
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, the environment variable
${bb_full_target}
within the build environment has the full concrete specification of the target, including C++ string ABI and libgfortran version, even if those details aren't relevant for the generated tarballs
While that is true, the Julia build system is smart enough to strip off the cxxabi
and libgfortran
tags for dependencies that don't care about them. The way Max is passing the target triplet in is correct, I am merely mentioning this for future-proofing.
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.
@giordano ah of course 🤦 that's what I was missing, thanks
UPDATE OK I guess I was missing even more 😂
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.
Yeah, I can of course change it to match something like -julia_version[^-]+
or so (except that it'll have to fit into a bash regex).
But since this works now, I hope we can merge this as-is, and I promise I'll soon submit a cleaned version, to future-proof this. But then other things that are waiting for this PR could already proceed now.
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.
Actually I just fixed this anyway, ${bb_full_target/-julia_version+([^-])}
should do it
86bddd2
to
63c9bcd
Compare
- fix FreeBSD builds - fix 32bit ARM builds - strip '-julia_version-1.X' from BB_TRIPLET_LIBGFORTRAN_CXXABI to fix downloads of some binaries (fixes e.g. macOS builds)
63c9bcd
to
6dd0162
Compare
Merge pretty please? 🥺 |
It is not clear to me right now why libjulia is not built for FreeBSD, so this PR is trying to figure it out, and then either solve it, or document it for future generations.