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

Toolchain+Meta+Nix: Make it build and run on darwin systems using nix #25728

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

iniw
Copy link
Contributor

@iniw iniw commented Feb 25, 2025

This PR adds the necessary changes to get Serenity building and running on darwin systems that utlize nix as their package manager and/or dev environment. I have only tested it on an Intel mac but it should work just fine on an ARM one.

The main change is statically linking the GNU toolchain. This is required because dynamically linking binutil's libctf causes issues on intel macs. The first commit's description goes more in detail about that and links some relevant issues/reports.
This change also requires a small "revert" of a LibC+LibELF patch that Andreas did a few years ago, since the "transaction memory" related stubs start causing ODR-related build failures when the toolchain is statically linked. From my (relatively thorough) testing this didn't change any previous behavior, but I would like someone more knowledgeable than me to double-check it.

The rest of the commits are hopefully pretty self-explanatory and agreeable.

The last commit is unrelated to the initial goal of the PR but is nix related so I decided to put it in as well. I can move it to a another PR if necessary but that would require an extra rebase after the first one (whichever one it ended up being) gets merged, since they touch the same parts of the code.

@iniw iniw requested a review from BertalanD as a code owner February 25, 2025 18:50
@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Feb 25, 2025
@timschumi timschumi self-requested a review February 25, 2025 19:14
@nico
Copy link
Contributor

nico commented Feb 26, 2025

I'll defer this to @timschumi who said he'll take a look.

Vaguely +1 to danshaders comment above. This doesn't seem necessary outside nix, and if it doesn't work in nix that feels more like a nix issue that needs a fix over there, instead of a downstream workaround in serenity. (I don't understand the problem fully, so maybe there's a good reason. But it feels like nobody else fully understands the problem yet either. So I think understanding what exactly the problem is would be a good first step.)

The du change is also a bit weird to me. It feels a bit like "i made malloc print to stdout instead of allocating memory; this updates all code in the world to no longer assume that malloc allocs memory". Does nix require making du GNU du? If not, could we just, well, not replace system du?

(But in the end I'm happy with whatever @timschumi is happy with.)

@timschumi
Copy link
Member

The du change is also a bit weird to me. It feels a bit like "i made malloc print to stdout instead of allocating memory; this updates all code in the world to no longer assume that malloc allocs memory". Does nix require making du GNU du? If not, could we just, well, not replace system du?

Nix aims to provide a somewhat hermetic environment, so the system du is not linked into it in the first place.

In theory we can modify the flake to link du to gdu so that we can continue checking the old path, but I don't see it as being less of an eyesore than checking if du (which in fact is the canonical name, despite what Homebrew says/does) is the GNU version.

The only note that I have regarding the du change is that, as far as I remember, gdu is also a name of a non-du tool that users might have installed, so checking if gdu is indeed GNU coreutils before proceeding was done on purpose.

Rest of review/testing will follow in the evening.

@nico
Copy link
Contributor

nico commented Feb 26, 2025

Nix aims to provide a somewhat hermetic environment, so the system du is not linked into it in the first place.

Maybe it shouldn't identify itself as "Darwin" then :) Making distinction between kernel and userland is a linux, uh, idea. Elsewhere, these are usually considered pretty coupled. Is darwin/nix detectable somehow? Maybe we could check !darwin && !nix?

@iniw
Copy link
Contributor Author

iniw commented Feb 26, 2025

Maybe it shouldn't identify itself as "Darwin" then :)

Well it is still a Darwin system, it uses apple's/xcode's sdk and libc.

The du change is also a bit weird to me. It feels a bit like "i made malloc print to stdout instead of allocating memory; this updates all code in the world to no longer assume that malloc allocs memory". Does nix require making du GNU du? If not, could we just, well, not replace system du?

Like I said on the discord server, my code doesn't replace the du binary that gets picked, it merely tries to make less assumptions about what flavor of du will run depending on your system.

@iniw
Copy link
Contributor Author

iniw commented Feb 26, 2025

I have changed the du commit to never even test for the existence of a gdu command and to correctly detect BusyBox-flavored coreutils.

This is a kind of cat and mouse game since it's not really possible to deal with all the possible permutations of dus out there.

As it stands it'll always use whatever the du in your $PATH happens to be and if it's GNU or BusyBox flavored it'll pick the -b flag. Otherwise it'll fallback to the BSD-compatible -A equivalent.

Copy link
Member

@timschumi timschumi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nitpicks, a practical test from me will have to wait until tomorrow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although I do wonder how this ever worked on SerenityOS, since we only appear to have the long form of the flag. Maybe it never did and I just misremember.

iniw added 3 commits February 26, 2025 23:16
This commit partially reverts 7db8ccc, which was added to remove a
DynamicLoader hack to ignore relocations for gcc's transactional
memory functionality, which Serenity didn't support.

Neither hack seem to be required anymore and the existing stubs
will cause ODR violations on the next commit, where the
GNU toolchain starts being statically linked.
This is done as an effort to support building Serenity on Intel macs,
which runs into link errors when dynamically linking binutil's libctf.

This is a known problem and has been reported on gnu's issue tracker
and on the spack package manager's github issues:

https://lists.nongnu.org/archive/html/bug-binutils/2023-11/msg00050.html
spack/spack#35817
spack/spack#42947

spack also "solves" it by simply *not* dynamically linking.

We even already had an in-tree hack to workaround a different, intl
related, linking error for Intel macs, which this patch allows us to remove.
The previous code assumed that, under a Darwin system, you would either
have the GNU coreutils version of `du` installed through brew, which
adds a 'g' prefix, or the default, BSD-like version.

This assumption can be easily broken by simply directly installing GNU's
coreutils or through a package manager that doesn't prepend a 'g' to it.

We now instead always use whatever `du` is in the user's $PATH and try
to detect which flavor it is and, consequently, which flag should be
used to retrieve the "apparent size" of something in disk.

The heuristic is not perfect and may lead to the wrong flag being
selected in edge cases, but it can always be amended in the future.
@iniw
Copy link
Contributor Author

iniw commented Feb 27, 2025

I have split the LibC change into it's own commit as per @DanShaders's request and have also improved the commit message for each change, which hopefully makes their motivations clearer and more contextualized. The whole PR should now make way more sense :)

@BuggieBot
Copy link
Member

Hello!

One or more of the commit messages in this PR do not match the SerenityOS code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 pr-needs-review PR needs review from a maintainer or community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants