This was fixed another way in master.

Yea, just rebased and dropped this.

This one really needs a proper solution rather sprinkling this buildPackages. in random places and being inconsistent with rest of Nixpkgs.

@dezgeg Hm what do you suggest? It's only used via string interpolation; it's not depended on in a way that can be put into nativeBuildInputs AFAIK. Also, it's not harmful to be more explicit about buildPackages, is it?

Ah oops, I was supposed to put the comment on the nativeBuildInputs = with pkgs.buildPackages; ... line.

But in general, this buildPackages. thing is bad since it breaks overriding inputs (like in callPackage ./foo { bar = baz; };) so it would need a replacement one day.

@dezgeg Ah, so I should take these inputs in the file's argument list and list them under nativeBuildInputs rather than maintaining the with pkgs; that was used before, right? TBH, I'm fairly confident I didn't need to add .buildPackages here....

I think this commit needs to go to staging.

Also gettext can be removed from nativeBuildInputs now.

Yea, I'll point this whole PR at staging; it's in no hurry to reach master.

Why can gettext be removed?

Because you put it in propagatedNativeBuildInputs it will apply to glib as well.

Ah, of course. Misread the first comment

@dezgeg Actually, this buildPackages was necessary; not sure why. This isn't a callPackage expression, but I thought that using pkgs.foo in nativeBuildInputs would be the same as pkgs.buildPackages.foo. Yet, with the former you get the cross version.

Yeah, exactly, derivations using nativeBuildInputs that aren't inside a callPackage are broken when cross-compiling and need this nativeBuildInputs = with buildPackages; [ ... ]; dance.

I think the solution would be have something like callPackagesInline that instead of a path to a Nix expression takes the Nix expression directly. So this would become something like:

system.build.sdImage = pkgs.callPackageInline ({ stdenv, dosfstools, e2fsprogs, mtools, libfaketime, utillinux }: 
      name = config.sdImage.imageName;

      nativeBuildInputs = [ dosfstools e2fsprogs mtools libfaketime utillinux ];
      /* and so on */
}):

@dezgeg callPackage can take a function instead of a file. But I'm not sure that's any more elegant than just being explicit...

Ah, indeed callPackage can take a function. That can be used then without any changes to other places. Well, except the other places that do this buildPackages hack need to be fixed up to use callPackage as well.

But I'm not sure that's any more elegant than just being explicit...

The fact that nativeBuildInputs works correctly in 99% of the cases (i.e. every package included from all-packages.nix) but silently does the wrong thing when not inside callPackage is really, really terrible. If adding a callPackage to those 1% cases makes them work like the 99% cases then that's the correct action. And then mkDerivation should be changed to throw an error when cross-compiling and not inside callPackage instead of silently doing the wrong thing.

That sounds like fighting a hack with a hack to me. The fact that callPackage is the solution here is a gross hack. There should be minimal relationship between mkDerivation and callPackage. callPackage just happened to be the easiest way to fix this for legacy code. To me it seems like the preference, if we'd started with cross compilation in mind, would have been to somehow be more explicit, not use this insane splicing that's hard to reason about. I don't think we're solving any problem by requiring the use of callPackage, we're just obscuring the code.

To me it seems like the preference, if we'd started with cross compilation in mind, would have been to somehow be more explicit, not use this insane splicing that's hard to reason about.

Yes, if someone had >10 years ago from the start decided that cross-compilation would be a thing to support and take it into the account in the design, things would have certainly been designed differently, no question about that. But hindsight is 20/20, and what we have now is what he have, and almost certainly is too late to change. Rewriting/redesigning interfaces for essentially a minority use case (and I really don't see the situation changing with the x86 monopoly for desktops and servers still being strong as ever) and breaking the existing code of the 90% of the userbase who have no need for cross compilation is not the correct tradeoff. (This is a personal opinion, but I don't think I'm the only one).

In other words, of the following choices:

a. Not supporting cross-compilation in Nixpkgs at all,
b. Supporting cross-compilation but with minimal possible changes to existing packages with compat tricks like splicing and separate buildInputs, PATH etc. logic in stdenv between native and cross cases,
c. Redesigning the entirety of nixpkgs around cross-compilation and changing each and single package,

I would pick b. any day, by a long shot (followed by a. then c.).

I don't think we're solving any problem by requiring the use of callPackage, we're just obscuring the code.

Actually, regardless of cross-compilation, using callPackage here would be an improvement because then you could use .override to change say, the version of e2fsprogs used here. Currently when I try it I just get something like error: attribute 'override' in selection path 'config.system.build.sdImage.override' not found which is inconsistent with all the rest of the 99% of the derivations in Nixpkgs.

Obviously I wasn't suggesting rewriting all of nixpkgs. I was saying that callPackage isn't holy in any way in this context. Native build input splicing isn't a great reason to use it when it's not necessary. In fact, I originally thought that splicing worked by mkDerivation checking the nativeBuildInputs and splicing those, not by splicing the package itself; that would have made a bit more sense to me, and would have made this fine.

Regardless, I hadn't thought about the desire to override this derivation normally. That's a good point. I've changed it to use callPackage, per your suggestion. Seems to work fine.

This doesn't look right. gettext is definitely a propagated build input. Dependents of glib will use it as a build input, not a native build input, so you won't get the propagated gettext when it's listed native here. I think the old thing was correct. Was there an issue with the previous thing?

gettext is both a lib used at runtime and a binary used to process source code, IIRC. So you need the native one for the binary. Not really sure what the deal with propagation is... Is there a test I can use to confirm how propagation of this dep ought to behave?

Yeah, gettext does have a binary but I don't think we want it to propagate through glib. I think it was a propagated build input originally because it depends on some headers in gettext. Unless it's breaking something I would leave it as is.

@matthewbauer Well, whatever build failure I experienced in past revisions of nixpkgs is now gone without this change, so I guess I'll remove it. @dezgeg Does that mean this PR no longer needs to target staging?

Hmm, yeah, if gettext comes with a tool that is used at build-time (I presume it's to parse all the source code of the project to find all the original translations of the strings), then that binary needs to be moved to the dev output. (The cross stdenv stille needs to be fixed to include such tools in PATH, though).

At least gettextize definitely belongs to dev. So that's something to fix at some point.

Yes, no longer need for staging if this glib change is dropped.

Consider moving the package function to a separate let name. I think it would clear up the code a bit due to callPackage ... {}.