Skip to content

arm-trusted-firmware: tell Makefile how our C compilers are named#247922

Merged
sternenseemann merged 3 commits intoNixOS:masterfrom
sternenseemann:arm-trusted-firmware-cc-naming
Aug 17, 2023
Merged

arm-trusted-firmware: tell Makefile how our C compilers are named#247922
sternenseemann merged 3 commits intoNixOS:masterfrom
sternenseemann:arm-trusted-firmware-cc-naming

Conversation

@sternenseemann
Copy link
Copy Markdown
Member

Description of changes

We already did this for stdenv.cc via CROSS_COMPILE, but neglected HOSTCC/CC_FOR_BUILD and the arm-embedded toolchain.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

arm-trusted-firmware assumes that HOSTCC is `gcc` which is not
necessarily the case.
Some of the firmware requires an arm-embedded toolchain to be in PATH.
The Makefile offers a similar mechanism to CROSS_COMPILE for letting it
know what naming scheme is used for the accompanying tools.
@sternenseemann sternenseemann requested a review from a user August 8, 2023 11:06
@ofborg ofborg bot requested a review from lopsided98 August 8, 2023 11:51
@ofborg ofborg bot added 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. labels Aug 8, 2023
Copy link
Copy Markdown
Contributor

@lopsided98 lopsided98 left a comment

Choose a reason for hiding this comment

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

Cross-compiled all targets, and tested native and cross build of armTrustedFirmwareTools.

@sternenseemann
Copy link
Copy Markdown
Member Author

The HOSTCC may actually be wrong, I'll look into it soon or rebase it out for now.

@lopsided98
Copy link
Copy Markdown
Contributor

In what way? I was concerned that it wouldn't be expanded correctly, but it seems to. On the other hand, I don't think any of the targets actually use it, and it gets overridden in armTrustedFirmwareTools, where we want to build everything for the host platform.

As explained in the new comment, we trick the build system here to build
its build tools for the host platform. To make this even more foolproof
/ reliable, stop adding CC_FOR_BUILD to the environment, so there can be
no mix up.
@sternenseemann
Copy link
Copy Markdown
Member Author

@lopsided98 Thanks, you are right! Got confused by precisely that trick. I added an explanatory comment and removed CC_FOR_BUILD from the environment of that derivation (just to be safe) in a new commit.

@ofborg ofborg bot requested a review from lopsided98 August 9, 2023 12:42
Copy link
Copy Markdown

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM but I'm currently unable to test this (beyond building it, which I did do).

Something in Binutils 2.39 or 2.40 broke arm-trusted-firmware's wake-from-sleep on RK3399. Nixpkgs aggressively deletes old binutils -- except for vendor toolchains, which I will probably be forced to switch to.

So I haven't been able to rebuild ATF for the last ~year 😡 . ATF is annoyingly fragile. I should probably just bite the bullet and accept that I have to use the vendor toolchains like normal people do. It's kinda sad that that is the only way to pin binutils on nixpkgs.

@sternenseemann sternenseemann merged commit b1981b0 into NixOS:master Aug 17, 2023
@sternenseemann sternenseemann deleted the arm-trusted-firmware-cc-naming branch August 17, 2023 07:57
@sternenseemann
Copy link
Copy Markdown
Member Author

Something in Binutils 2.39 or 2.40 broke arm-trusted-firmware's wake-from-sleep on RK3399. Nixpkgs aggressively deletes old binutils -- except for vendor toolchains, which I will probably be forced to switch to.

We could re-introduce a version in principle, although overriding the binutils version used to build something is a bit finnicky.

@ghost
Copy link
Copy Markdown

ghost commented Aug 19, 2023

overriding the binutils version used to build something is a bit finnicky.

I've always wondered why that is so difficult. Is there some fundamental reason why that is the case?

Being able to overrideBinutils the same way we can overrideCC is a need that I encounter quite frequently. I know @RaitoBezarius has a pressing need for it.

@sternenseemann
Copy link
Copy Markdown
Member Author

I don't see a reason why it shouldn't be possible. It's just more finnicky since the bintools wrapper needs to be passed to the cc wrapper.

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.

2 participants