Skip to content

limine: Fix build#377696

Merged
GaetanLepage merged 1 commit intoNixOS:masterfrom
boomshroom:patch-1
Feb 22, 2025
Merged

limine: Fix build#377696
GaetanLepage merged 1 commit intoNixOS:masterfrom
boomshroom:patch-1

Conversation

@boomshroom
Copy link
Contributor

Resolves #377469 by compiling the freestanding part with an unwrapped compiler.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@boomshroom boomshroom changed the base branch from nixos-unstable to master January 29, 2025 07:02
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 29, 2025
Copy link
Member

@lzcunt lzcunt left a comment

Choose a reason for hiding this comment

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

freestanding-toolchain assumes LLVM toolchains are in PATH so it should be patched to support LLVM toolchains not in path. As a workaround you can try passing all of AR_FOR_TARGET, CC_FOR_TARGET, LD_FOR_TARGET, OBJCOPY_FOR_TARGET, OBJDUMP_FOR_TARGET, READELF_FOR_TARGET.

@surfaceflinger
Copy link
Member

unwrapped clang shouldn't do stuff like adding architecture related args like wrapped one does, right? I wonder if this could also fix building on Darwin

@lzcunt
Copy link
Member

lzcunt commented Jan 29, 2025

unwrapped clang shouldn't do stuff like adding architecture related args like wrapped one does, right? I wonder if this could also fix building on Darwin

By architecture related args if you mean --target no that's staying, that's added by limine's build system but stuff like hardening flags would not be added.

I haven't tested but this could also remove the need for disabling zerocallusedregs (I think cc-wrapper adds that in?) but we might consider adding supported hardening flags manually since likely all of them will stop functioning with the unwrapped clang.

@surfaceflinger
Copy link
Member

By architecture related args if you mean --target no that's staying, that's added by limine's build system but stuff like hardening flags would not be added.

I mean I had access to a macbook for few hours in October so I tried to get it to build and as far as I remember it was because wrapper was adding march or something which was conflicting with --target. I might be wrong tho and I can't really verify this anymore

Copy link
Member

@lzcunt lzcunt left a comment

Choose a reason for hiding this comment

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

Changes are good but freestanding-toolchain's checks fail when cross compiling (aka enableAll is broken)

@boomshroom
Copy link
Contributor Author

boomshroom commented Feb 19, 2025

Ok, so looking at Limine's build process, either CC_FOR_TARGET has to be able to build every desired target without any flags, or clang on the PATH has to not generate a warning when being passed -target. This means that an unwrapped clang toolchain has to be the default on the PATH, and there don't appear to be any options to set that clang's location through an environment variable. If we want different flags for each target when using --enable-all, then we cannot be setting CC_FOR_TARGET.

Except the unwrapped clang can't find the C runtime libraries, so configure aborts before any of that can get checked. At this point, I'm tempted to just put up with the warning and cut the first line from each toolchain file. Or I suppose we can patch the script to accept a different clang.

I'm not sure if --enable-all is really worth it.

@boomshroom
Copy link
Contributor Author

I'm ashamed of what I had to do to work around this:

    if [ -z "$FREESTANDING_CC" ] && [ "x$WANT_FREESTANDING_CC" = "xyes" ]; then
        _FREESTANDING_CC="clang -target $TRIPLET"
        validate_cc "$_FREESTANDING_CC" || break
    fi

@phip1611
Copy link
Member

phip1611 commented Feb 19, 2025

I'm not sure if --enable-all is really worth it.

Not for NixOS itself when it wants to use Limine as bootloader for itself. But it is important for OS developers that want to create bootable images for various systems for their custom kernels/custom OS.

Thanks for putting time and effort into this!

@boomshroom boomshroom requested a review from lzcunt February 19, 2025 22:04
Copy link
Member

@lzcunt lzcunt left a comment

Choose a reason for hiding this comment

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

The patch looks good, maybe consider squashing the second commit because it's a fixup.

Thanks for the patch! Hopefully we can update to 9.0 after this is merged

Copy link
Member

@surfaceflinger surfaceflinger left a comment

Choose a reason for hiding this comment

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

Everything works exactly as it did before it broke, thanks!

Resolves NixOS#377469 by compiling the freestanding part with an
unwrapped compiler. This compiler is required to be the default
directly in the `PATH`. However a *wrapped* compiler is *also*
required, but can be passed by a `CC` environment variable.

Includes change to clang path from lzcnt.
@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 377696


x86_64-linux

✅ 4 packages built:
  • limine
  • limine.dev
  • limine.doc
  • limine.man

aarch64-linux

✅ 4 packages built:
  • limine
  • limine.dev
  • limine.doc
  • limine.man

Copy link
Contributor

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

LGTM thanks

@GaetanLepage GaetanLepage merged commit d08c35b into NixOS:master Feb 22, 2025
25 of 27 checks passed
@boomshroom boomshroom deleted the patch-1 branch February 22, 2025 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build failure: limine

5 participants