Replace build wrapper with native meson targets#1722
Conversation
|
Build failed. ✔️ unit-test SUCCESS in 2m 02s |
PeakKS
force-pushed
the
redo-build-system
branch
from
88bdd17 to
a4c63b1
Compare
|
Build failed. ❌ unit-test FAILURE in 1m 34s |
|
Build failed. ❌ unit-test FAILURE in 1m 34s |
|
Not sure how CI is failing, logs seem to show the binary getting compiled then not existing for completions? |
debarshiray
left a comment
debarshiray
left a comment
There was a problem hiding this comment.
Thanks for working on this, @PeakKS ! I don't have time right now to take a very close look, but I didn't want to completely ignore your significant contribution. So some fly-by comments:
First, I think what you are suggesting is a good idea. The less shell scripts we use, the better it is! :)
Second, do you think it will be possible to have tests to check if the toolbox binary was correctly built? You have already seen what src/go-build-wrapper was doing, and you will see the background in the Git history of that file. The toolbox binary needs to be linked in a very specific way for different Toolbx containers to work across different host OSes.
If we have tests to check the important attributes of the binary, then it becomes a lot easier to make changes to the build, and review those changes, etc.. It's one of the big reasons why we never revamped src/go-build-wrapper before. Manually checking everything felt too nerve-wracking and stressful. :)
debarshiray
left a comment
debarshiray
left a comment
There was a problem hiding this comment.
Could you please use your full name and a real email address in your Git authorship information?
The Signed-off-by trailer has no meaning without a real full name and email address.
I could have made an exception for a one line change because it would be legally insignificant. However, that's not the case with this pull request, because you have done a substantial amount of great work. :)
We need a real full name and email address for tracking copyright holders in case a licensing issue crops up. Note how projects as diverse as GCC, GnuPG, Linux and Podman don't allow anonymous or pseudonymous contributions.
PeakKS
force-pushed
the
redo-build-system
branch
from
1b925a9 to
fd565e8
Compare
No problem, didn't even realize I used my username lol. |
|
Build failed. ❌ unit-test FAILURE in 1m 37s |
|
Hey @PeakKS ! How is it going with this pull request. I am asking because I see that the CI is failing. Do you need some help? |
I haven't worked on this in a while but I think it's correct besides the CI. I can rebase on main to see if that magically fixes the CI, otherwise I'll need someone to take a closer look. |
Signed-off-by: Blake Batson <bbatson101@gmail.com>
Signed-off-by: Blake Batson <bbatson101@gmail.com>
Signed-off-by: Blake Batson <bbatson101@gmail.com>
Signed-off-by: Blake Batson <bbatson101@gmail.com>
Signed-off-by: Blake Batson <bbatson101@gmail.com>
|
Okay seems like the same issue as before. Links target? Yep: I also tried running my local build of toolbox to run a fedora42 container and try building there to replicate the CI build failure but it still builds fine inside. @debarshiray could you take a look? |
|
This is how the CI fails on Ubuntu hosts: As far as I can make out, step 12 generates |
debarshiray
left a comment
debarshiray
left a comment
There was a problem hiding this comment.
It's not immediately clear to me why the build breaks on the CI environment, because, like you, I can't reproduce it on my local system either. Meanwhile, here's an idea on how to move this forward:
|
|
||
| libc_path = run_command(cc_exe, '--print-file-name=libc.so', capture: true, check: true).stdout() | ||
| libc_canon = run_command(readlink, '--canonicalize', libc_path, capture: true, check: true).stdout() | ||
| libc_dir = run_command(dirname, libc_canon, capture: true, check: true).stdout().strip() |
There was a problem hiding this comment.
I like this approach of using Meson's run_command() to move things out of the go-build-wrapper shell script into Meson.
It seems to me that these parts can be split into a simpler commit that's less likely to break the CI.
| dynamic_linker = '/lib/ld-linux-riscv64-lp64d.so.1' | ||
| else | ||
| host_machine_description = cpu_family + ' (' + endian + ' endian)' | ||
| error('Please specify dynamic linker for:', host_machine_description) |
There was a problem hiding this comment.
I am curious about the switch from this to objcopy(1) to set the dynamic linker. In the past we used patchelf(1) to do the same thing, and we dropped it because some downstream distributors decided against shipping patchelf(1).
I see that objcopy(1) is part of GNU Binutils. So, it's more likely to be present than patchelf(1), but there's still benefit in the general principle of fewer build requirements within reason.
I found some tests hidden in #1031 Let's see if I can rescue them. |
2 participants
Tentative fix for #1706
Replaces the go build wrapper and the completions wrapper with just doing the build right in meson. Essentially I am just tricking go into building as a static library, then linking that with itself. This prevents the weird go build cache issue I ran into in #1706.
Since
-z nowcan not be used for this application, I am forcing-z lazyin the linker args. It seems like the reason this only popped up on gentoo is because we default to-z nowfor security reasons (see : https://packages.gentoo.org/useflags/default-znow).Getting rid of some of the boilerplate is just a nice side benefit :)