uboot: only apply raspberry pi patches to raspberry pi builds

[lheckemann]

Motivation for this change

Otherwise, builds which don't care about the raspberry pi at all may
fail to build because the rpi-specific patches don't apply to that
particular source.

Found while trying to build mobile-nixos's pinephone uboot for the currently pinned nixpkgs revision.

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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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/)
• 21.11 Release Notes (or backporting 21.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.

[samueldr]

(As briefly stated in the NixOS on ARM matrix room)

The buildUBoot interface has poor semantics for being used outside of Nixpkgs safely.

Moving the patches around is not good. We should dogfood patches entirely, proving it's consequences-free on all tested platforms, rather than hope it is and only patch one platform.

The solution I would prefer being used is an interface that allows disabling the built-in patch set. Though it might be "too much magic", it could apply the patches only if src is defaultSrc.

[samueldr]

(Side-note: there are two fresh competing PRs to fix the Pinephone issues in Mobile NixOS already)

[lheckemann]

I think we shouldn't be applying any patches (unless they're for buildability with nixpkgs, but we don't have any such patches and if we need them they should be upstreamed) in the generic buildUBoot function. And since these patches only affect raspberry-pi-specific code, I think it makes sense to move them to the raspberry-pi-specific uboot builds.

[FlorianFranzen]

While this might not be what we want in the end, it is better then the current solution which applies these patches to all uboot builds,

[samueldr]

The main issue here is that buildUBboot is assumed to be an external interface, while in reality it is an internal interface being exposed. It may have been exposed such that callPackage was able to provide buildUBboot to the few U-Boots that were once defined in external files. It is from before I started maintaining U-Boot.

We (overall, not we people in this discussion) had the discussion about applying patches in the past, and the preferred option was to always apply patches all the time, to better dogfood them, rather than accidentally cause partial build failures.

I understand we need a solution to this issue, but I really believe this is not the way forward. Mainly because this is pushing aside the real issue that at some point in the future we may need to apply "for Nixpkgs" patches, and at that point the issue comes back.

[lheckemann]

at some point in the future we may need to apply "for Nixpkgs" patches, and at that point the issue comes back.

I'm perfectly fine with "for nixpkgs" patches breaking my out-of-nixpkgs build though. Less so with "for raspberry pi" patches breaking my non-raspberry-pi build. I think that even if buildUBoot isn't intended as an external interface, it should still be maximally useful for building other u-boots --- regardless of whether it's used outside nixpkgs or within nixpkgs in the future.

[ck3d]

Since the function is called buildUboot and not buildPiUboot I appreciate the PR. The interface to not allow to disable the patches.
Could you please rebase the PR?

[sielicki]

The main issue here is that buildUBboot is assumed to be an external interface, while in reality it is an internal interface being exposed. It may have been exposed such that callPackage was able to provide buildUBboot to the few U-Boots that were once defined in external files. It is from before I started maintaining U-Boot.

I feel it's useful and should be kept around. Being able to follow the instructions here to get a uboot binary immediately without any concern around figuring out dependencies or cross compilers is hugely useful and reduces barriers to people interested in porting new boards.

To that end, this patch gets in the way of anyone who wants to build against upstream uboot, or some vendor fork, or in-tree with a flake with src = ./. ;, and at the very least they should have an escape-hatch without being forced to modify their nixpkgs pointer to modify the function. There have now been two separate PRs around this because people have been in this situation.

We (overall, not we people in this discussion) had the discussion about applying patches in the past, and the preferred option was to always apply patches all the time, to better dogfood them, rather than accidentally cause partial build failures.

Fair concern, but particularly for the rpi, which is (guessing here) by-far the most common arm board that people are using, I am not sure it will go unnoticed for long.

I understand we need a solution to this issue, but I really believe this is not the way forward. Mainly because this is pushing aside the real issue that at some point in the future we may need to apply "for Nixpkgs" patches, and at that point the issue comes back.

IMO the preference should always be to not apply patches unless absolutely necessary and I'm not sure it's right to plan for a prolonged period of needing to apply "for nixpkgs" patches -- for this particular patch, upstream probably won't take it, but in other cases I think there's a strong likelihood that if nixos needs it, they'll just take the change straight-up.

For this particular patch, maybe upstream would accept a patch that does:

#ifndef ENV_MEM_LAYOUT_SETTINGS
#define ENV_MEM_LAYOUT_SETTINGS [...]
#endif

And then we can pass it from make?