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

lib.systems.examples: use four component triples for embedded #247490

Open
wants to merge 5 commits into
base: staging
Choose a base branch
from

Conversation

sternenseemann
Copy link
Member

Description of changes

Should solve #165836 by using triples that are interpreted the same by nixpkgs and the current gnu-config version.
Depends on/includes changes from #247325.

Tested stdenv build for:

  • riscv32
  • riscv64
  • misp64
  • mips
  • rx
  • arm eabi
  • arm eabihf
  • aarch64_be
  • powerpc
  • powerpcle

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.

@github-actions github-actions bot added 6.topic: stdenv Standard environment 6.topic: lib The Nixpkgs function library labels Aug 6, 2023
@ofborg ofborg bot added the 10.rebuild-linux-stdenv This PR causes stdenv to rebuild label Aug 6, 2023
@ofborg ofborg bot requested review from dezgeg and emilytrau August 6, 2023 11:32
@Ericson2314
Copy link
Member

Oh yay! I had no idea whether more changes (e.g. to GCC) would be needed.

@sternenseemann sternenseemann changed the base branch from master to staging August 6, 2023 18:22
Thanks to a [change in gnu-config] we no longer have to rely on
gnu-config interpreting `none` as a vendor and nixpkgs as a kernel.
Instead we can specify a four component triple that corresponds to how
nixpkgs interprets the shortened one because gnu-config now accepts
them (the long form at least).

As a result, we also need to make sure that an up to date gnu-config is
used for the affected platforms (everything with kernel none).

Reference NixOS#165836.

[change in gnu-config]: https://git.savannah.gnu.org/cgit/config.git/commit/?id=998ba1414387b4ce1a519be234e1609bc7912e0c
@sternenseemann sternenseemann force-pushed the no-more-ambiguous-embedded-triples branch from 6b26aaa to 8fec22f Compare August 6, 2023 18:23
@sternenseemann sternenseemann marked this pull request as ready for review August 6, 2023 18:23
@sternenseemann sternenseemann requested a review from a user August 6, 2023 18:23
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 and removed 10.rebuild-linux-stdenv This PR causes stdenv to rebuild 10.rebuild-darwin: 11-100 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ labels Aug 6, 2023
@ghost
Copy link

ghost commented Aug 6, 2023

Do we really need to change nixpkgs' triples for these platforms? Changing our triples for these platforms will be awkward for anybody already using them.

#165836 is fixed by #235230 with the added benefit of not requiring that we always use most-canonical triples.

If we're going to require always-most-canonical triples we should do that everywhere, not just for certain platforms.

@ghost
Copy link

ghost commented Aug 6, 2023

Also we need to make sure that we aren't breaking "blessed precompiled" vendor toolchains like gcc-arm-embedded -- we can't change their choice of triple.

This PR needs to be considered very carefully. Please don't rush forward with this.

@Ericson2314
Copy link
Member

I lean heavily towards this approach. If we discover further issues I'm happy to fix upstream packages further.

To me the basic truth is that if we don't proactively weed out the accidental complexity that is out there, no one else will. So yes we're pioneering, and there are risks, but someone has got to.

@ghost
Copy link

ghost commented Aug 7, 2023

I'm happy to fix upstream packages further.

How are you going to fix the precompiled binaries that ARM publishes?

Are you volunteering to fix all the breakage from moving all of our platforms (including x86_64-linux) to fully canonical triples?

This has not been thought through.

@sternenseemann
Copy link
Member Author

Changing our triples for these platforms will be awkward for anybody already using them.

How so? lib.systems.examples is intended as a way to use a platform abstract from the magic incantation required to assemble nixpkgs targeting it. If you have to hardcode the config anywhere, something is wrong somewhere would be my inclination.

Also we need to make sure that we aren't breaking "blessed precompiled" vendor toolchains like gcc-arm-embedded -- we can't change their choice of triple.

It is still possible to pass a different config. Also targetPrefix and ${platform.config}- don't have to be the same, so a precompiled toolchain should by and large still work (or could be easily adapted).

The way I see lib.systems.examples are our toolchains and we are within our rights to establish our own canonicality, we are likely one of the few able to do this well due to the amount of ground we cover. Taking vendor toolchains as a source of truth seems like a bad idea, since they are kind of bad at picking a sensible triple.

@ghost
Copy link

ghost commented Aug 7, 2023

lib.systems.examples is intended as a way to use a platform abstract from the magic incantation required to assemble nixpkgs targeting it. I

Yes, and people are already using that incantation, via pkgsCross. Even nixpkgs itself uses pkgsCross.arm-embedded in arm-trusted-firmware. Did you test that? Did you test anything other than stdenv?

Taking vendor toolchains as a source of truth seems like a bad idea, since they are kind of bad at picking a sensible triple.

We already picked the triple for arm-embedded, a long time ago. This is about stability. Also since this is a breaking change why doesn't this PR update the release notes? What is the benefit of this breaking change? Why is that benefit worth the cost?

we are within our rights to establish our own canonicality

Then why not change all triples to be canonical? Including x86_64-linux-gnu, which we currently use and which is not canonical.

Come on, this PR is ridiculous, you make it out to be about getting rid of the noncanonical triples but it doesn't do that. What it really does is paper over the flaws in our triple parser, by sweeping a few (but not all) of the cases where it is broken under the rug. That's poor engineering and not something to be proud of. We should fix the bugs in the triple parser rather than cause random downstream breakage simply to evade those bugs.

@Ericson2314
Copy link
Member

Ericson2314 commented Aug 7, 2023

This PR is not papering over things. 3 triples are ambiguous, and even if we always parsed them the exact way as GNU config, GNU config is not the only source of truth either. I would rather simply more aggressively reject ambiguous things in the triple parser --- if we don't accept something then are not biassing LLVM over GNU config or vice versa.

I made the changes to GNU config with the sense that resolved the issue --- we were out of sink with upstream, and now upstream is changed. Now discussing the vendor toolchain triples feels like moving the goal posts, to be honest. Since when have prebuilt binaries been the source of truth about anything with Nixpkgs? The explicit goal of all our work, including the recent pure bootstrap work, is to have complete control by building the whole world from source. I don't want to give that up on the policy level.

Yes, ARM is the hardware manufacture, but I see 0 evidence that the triple they use is carefully chosen. The entire history of hardware is everyone making dubious decisions that are expedient in the moment but the introduce lots of accidental complexity. Why would the choice of triple be any different? It seems obvious to me that some engineer at ARM simply when with the first thing that worked, and this might have predated 4-component configs too.

I might be able to in fact contact an engineer who works on compilers at ARM. Would that help assuage you that we are doing nothing sketchy here?

@ghost
Copy link

ghost commented Aug 7, 2023

3 triples are ambiguous

Of course not. Every valid 3-triple canonicalizes to exactly one canonical triple. config.sub is the totally unambiguous specification for this mapping.

Since when have prebuilt binaries been the source of truth about anything with Nixpkgs?

You misunderstand. We're not picking brand new triples here. This is about not breaking a decision we've already made. That decision happened to be made in a way that allowed people on embedded platforms to use vendor toolchains, so they've been using them. We shouldn't break that without warning.

I might be able to in fact contact an engineer who works on compilers at ARM. Would that help assuage you that we are doing nothing sketchy here?

Since they probably don't own a time machine, no it wouldn't. You've seriously misunderstood my point here. I'm not looking to them as a source of authority. I'm pointing out that we made a decision that allowed downstream to use these toolchains, so we shouldn't break that.

I'm going to propose a compromise in my next comment.

Copy link

@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.

Instead of pulling the rug out from underneath anybody using these pkgsCross.* attributes, let's lay down a new rug and ask them to relocate themselves.

Specifically, let's split your change into three separate steps:

  1. Add new lib.examples.* entries for the new triples you want.
  2. Deprecate the old entries.
  3. Remove the old entries.

I have absolutely no problem with # 1; let's do it right now, today.

I'm less easily convinced of 2 and 3, but in any case since this is lib.*, which is the rare part of nixpkgs that is meant to be a stable public interface, changing things there requires lib.warn deprecation cycles anyways, so there's no rush.

We shouldn't pull the rug out from under people using these entries by silently changing their meaning. If you want to migrate people to new triples, this is the way it has to be done.

(Note: I did not carefully test the exact changes below, because git-hub doesn't let you upload a commit as a suggested change. If you like this compromise and apply these, give me a chance to test them.)

@@ -124,22 +124,22 @@ rec {
riscv32 = riscv "32";

riscv64-embedded = {
config = "riscv64-none-elf";
config = "riscv64-unknown-none-elf";
Copy link

Choose a reason for hiding this comment

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

Suggested change
config = "riscv64-unknown-none-elf";
config = "riscv64-none-elf";
libc = "newlib";
};
riscv64-embedded-canonical = {
config = "riscv64-unknown-none-elf";
libc = "newlib";
};

libc = "newlib";
};

riscv32-embedded = {
config = "riscv32-none-elf";
config = "riscv32-unknown-none-elf";
Copy link

Choose a reason for hiding this comment

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

Suggested change
config = "riscv32-unknown-none-elf";
config = "riscv32-none-elf";
libc = "newlib";
};
riscv32-embedded-canonical = {
config = "riscv32-unknown-none-elf";
libc = "newlib";
};

libc = "newlib";
};

mips64-embedded = {
config = "mips64-none-elf";
config = "mips64-unknown-none-elf";
Copy link

Choose a reason for hiding this comment

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

Suggested change
config = "mips64-unknown-none-elf";
config = "mips64-none-elf";
libc = "newlib";
};
mips64-embedded-canonical = {
config = "mips64-unknown-none-elf";
libc = "newlib";
};

libc = "newlib";
};

mips-embedded = {
config = "mips-none-elf";
config = "mips-unknown-none-elf";
Copy link

Choose a reason for hiding this comment

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

Suggested change
config = "mips-unknown-none-elf";
config = "mips-none-elf";
libc = "newlib";
};
mips-embedded-canonical = {
config = "riscv64-unknown-none-elf";
libc = "newlib";
};

@@ -153,7 +153,7 @@ rec {
};

rx-embedded = {
config = "rx-none-elf";
config = "rx-unknown-none-elf";
Copy link

Choose a reason for hiding this comment

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

Suggested change
config = "rx-unknown-none-elf";
config = "rx-none-elf";
libc = "newlib";
};
rx-embedded-canonical = {
config = "rx-unknown-none-elf";
libc = "newlib";
};

@@ -204,22 +204,22 @@ rec {
};

aarch64-embedded = {
config = "aarch64-none-elf";
config = "aarch64-unknown-none-elf";
Copy link

Choose a reason for hiding this comment

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

Suggested change
config = "aarch64-unknown-none-elf";
config = "aarch64-none-elf";
libc = "newlib";
};
aarch64-embedded-canonical = {
config = "aarch64-unknown-none-elf";
libc = "newlib";
};

libc = "newlib";
};

aarch64be-embedded = {
config = "aarch64_be-none-elf";
config = "aarch64_be-unknown-none-elf";
Copy link

Choose a reason for hiding this comment

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

Suggested change
config = "aarch64_be-unknown-none-elf";
config = "aarch64_be-none-elf";
libc = "newlib";
};
aarch64_be-embedded-canonical = {
config = "aarch64_be-unknown-none-elf";
libc = "newlib";
};

libc = "newlib";
};

ppc-embedded = {
config = "powerpc-none-eabi";
config = "powerpc-unknown-none-eabi";
Copy link

Choose a reason for hiding this comment

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

Suggested change
config = "powerpc-unknown-none-eabi";
config = "powerpc-none-elf";
libc = "newlib";
};
ppc-embedded-canonical = {
config = "powerpc-unknown-none-elf";
libc = "newlib";
};

libc = "newlib";
};

ppcle-embedded = {
config = "powerpcle-none-eabi";
config = "powerpcle-unknown-none-eabi";
Copy link

Choose a reason for hiding this comment

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

Suggested change
config = "powerpcle-unknown-none-eabi";
config = "powerpcle-none-elf";
libc = "newlib";
};
powerpcle-embedded-canonical = {
config = "powerpcle-unknown-none-elf";
libc = "newlib";
};

@@ -80,7 +80,7 @@ in lib.init bootStages ++ [
(hostPlatform.isLinux && !buildPlatform.isLinux)
[ buildPackages.patchelf ]
++ lib.optional
(let f = p: !p.isx86 || builtins.elem p.libc [ "musl" "wasilibc" "relibc" ] || p.isiOS || p.isGenode;
(let f = p: !p.isx86 || builtins.elem p.libc [ "musl" "wasilibc" "relibc" ] || p.isiOS || p.isGenode || p.isNone;
Copy link

Choose a reason for hiding this comment

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

Suggested change
(let f = p: !p.isx86 || builtins.elem p.libc [ "musl" "wasilibc" "relibc" ] || p.isiOS || p.isGenode || p.isNone;
(let f = p: !p.isx86 || builtins.elem p.libc [ "musl" "wasilibc" "relibc" ] || p.isiOS || p.isGenode || (p.vendor=="unknown" && p.isNone);

Copy link

Choose a reason for hiding this comment

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

This is the change that most needs to be double-checked. I'll do that if the general approach is agreeable to you.

@Ericson2314
Copy link
Member

I am OK with the general direction of the compromise, but there is something that needs to be cleared up:

  1. I called that file examples.nix because I intended it to be just that: mere examples of usage. I wanted people to write down what they wanted from scratch, using the built-in mechanisms to default missing fields to make that ergonomic.
  2. pkgsCross was added, which became very popular, but IMO is done totally wrong: only release.nix should be enumerating/tabulating known-good configurations. Nixpkgs itself should only know about the current configuration. Making pkgsCross part of Nixpkgs such that other packages can refer to it is incorrect.

I get that in the absense of clear signals, things can become de-facto stable in ways that no one planned, but if are going to take steps because this stuff has ossified, we should clean up a few other things to reflect that:

  1. Don't call a stable thing "examples"
  2. maybe try to move this stuff out of lib as it is far more Nixpkgs-specific than the rest of lib (that was my mistake all long)
  3. maybe deprecate pkgsCross and make a different thing for accessing pre-packaged package sets.

BTW in the past, I and others did change things in example.nix quite willy nilly, as old kludges could be removed. Maybe the era on that has closed, but I thought i should mention it.

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.
@ofborg ofborg bot requested a review from lopsided98 August 8, 2023 00:03
@sternenseemann
Copy link
Member Author

arm-trusted-firmware

…is fixed. The problem there was arguably a packaging bug, the expression neglected to pass the compiler name along via M0_CROSS_COMPILE (which is not very discoverable, granted). Seems like arm is also not too strict about the name :)

breaking change

Yes, a release notes entry doesn't hurt—I'll write one.

poor engineering

No amount of engineering can solve a fundamentally unsolvable problem.

Every valid 3-triple canonicalizes to exactly one canonical triple.

Firstly, this would require gnu-config as the absolute source of true, but even then it is not true, as the canonicalization changes over time. We already have to support triples that are not accepted by gnu-config and there is no telling how and if they are going to integrate them.

To my earlier point…

Also targetPrefix and ${platform.config}- don't have to be the same, so a precompiled toolchain should by and large still work (or could be easily adapted).

… I can also add that autotools actively discourages using the triple for any kind of configure tasks, recommending compilation probes instead (which are more reliable). The kind of wildcard matching they say you can do in a pinch would in most cases not catch the difference between arm-none-eabi and arm-unknown-none-eabi—because it is unreliable and ambiguous stuff to begin with. I guess it's most important role is to determine whether we are cross compiling in configure scripts (which is kind of bad for nixpkgs).

@sternenseemann
Copy link
Member Author

Looking at gcc-arm-embedded, it seems to me that it is not even using cc-wrapper and is not integrated into the cross infrastructure at all.

@ofborg ofborg bot requested review from blitz and kliu128 August 8, 2023 13:27
@ghost
Copy link

ghost commented Aug 8, 2023

I am OK with the general direction of the compromise, but there is something that needs to be cleared up:

1. I called that file `examples.nix` because I intended it to be just that: mere _examples_ of usage. I wanted people to write down what they wanted from scratch, using the built-in mechanisms to default missing fields to make that ergonomic.

2. `pkgsCross` was added, which became very popular, but IMO is done totally wrong: only `release.nix` should be enumerating/tabulating known-good configurations. Nixpkgs itself should only know about the current configuration. Making `pkgsCross` part of Nixpkgs such that other packages can refer to it is incorrect.

I very strongly agree with all of this.

If we had a way to accomplish the "memoisation" that pkgsCross provides, I would immediately start working on a PR to delete examples.nix. If somebody figures out how to memoize the cross-compiled packagesets without pkgsCross, let me know.

@ghost
Copy link

ghost commented Aug 8, 2023

I'd like to clear the air a bit here.

I poured an insane amount of hours into #235230, which (a) fixes our triple-parser and (b) establishes a test suite so it stays fixed. This was @sternenseemann's idea (although he originally suggested doing it by writing a bash interpreter in Nix, which I elected not to do). I picked up that idea and ran with it, all the way to the finish line, at considerable personal cost.

@sternenseemann and @Ericson2314, both of you seem to have activately ignored that PR. That's not such a big deal. However with this PR #247490 (and #247922 and #247923) you both seem to be going to incredible lengths to Frantically Cobble weird whack-a-mole workarounds for all the bugs that are already fixed cleanly by #235230. That really, really bothers me. I can handle being ignored, and I take pride in being able to stand down when somebody proposes a better solution than mine. But that's not what's happened here. You two are going out of your way to piece together ugly kludges in order to avoid having to merge or even review #235230.

I have no idea why you are going to such great lengths to do this. It is weird. But I'll certainly keep that in mind the next time I'm tempted to work on one of sterni's ideas. I feel like I got bamboozled pretty badly there.

@Ericson2314
Copy link
Member

@amjoseph-nixpkgs

[I realize we've had a bunch of pleasant conversation and collaboration more recently, but your post about from August still deserves a reply, so here it goes.]

So the way I see is that the current parser except things which are not normal forms, just as GNU config does. Your PR adds testing, which is fantastic! But it also:

  • Makes more normal forms that we do need need for expressive power (they are redundant normal forms)
  • Add a lot complexity handling various special cases

Neither of this these things I like. I specifically want Nixpkgs to not support everything GNU config supports, but GNU config I believe supports too many things in a chaotic way; I don't think they would even disagree with that, but rather admit it is the case but say they have little choice because they are bound by 3 decades of back compat. Our parser is much newer, and I do not believe we bound by such decades of weight, so we should strive to come up with something simple and more elegant.

They way I would like to do that is this PR. I think we can change the configs in exmples.nix, I do not think think we've made any progress that they do not change. In fact, using your tests we should ensure going forward that the configs in examples.nix are always normal forms, so this sort of problem does not occur again.


BTW here is one way to think about prioritizing simplicity. In issues like #242336, I've promised @trofi something "look, stuff might seem crazy right now as we do things in weird ways, but once we've cleaned out all the major tech debt I promise it will be simpler than it was before". But if we just do #235230 alone, I am breaking that promise --- because there is no timeline for GNU config to drop its legacy cruft, and because we committing to matching a larger and more chaotic subset of it, we are permanently resigning ourselves to more complexity.

Therefore, to ensure the continued acceptance of your (and hopefully our, once I find the time :)) work overhauling how GCC is packaged and bootstrapped, which I consider among the most foundational important efforts going on with Nixpkgs for a variety of reasons (the interaction with the minimal bootstrap among them), I feel compelled to mind the complexity budget elsewhere. Ensuring that we don't dramatically increase the lib.systems.parser complexity seems to me like a good way to preserve that budget.


There is one triple in examples.nix I am no longer trying to change, however, and that is the MinGW configs. After https://git.savannah.gnu.org/cgit/config.git/commit/?id=28ea239c53a2d5d8800c472bc2452eaa16e37af2 the decision from GNU config is loud and clear, and therefore our MinGW configs in example.nix should stay the same, and @amjoseph-nixpkgs commit changing the normal form to not be windows-gnu is exactly correct. I previously singled it out as the first one #235230 I wanted different, now in light of these events I am doing a 180 and saying we definitely should accept it!

@Ericson2314
Copy link
Member

@sternenseemann Also in light of https://lists.gnu.org/archive/html/config-patches/2023-09/msg00048.html I think this PR should also do vc4-elf -> vc4-unknown-none-elf. We can (a) apply that patch (b) no longer prevent overriding config.sub for that GCC, and then everything should work along with us having one less special case.

@ghost
Copy link

ghost commented Oct 30, 2023

Our parser is much newer, and I do not believe we bound by such decades of weight, so we should strive to come up with something simple and more elegant.

I think the parser is just an implementation detail. The taxonomy is the real question.

Are we trying to invent a new taxonomy? I had hoped to avoid that.

@Ericson2314
Copy link
Member

Invent a simpler sub-taxonomy. Everything we accept, GNU Config will accept with the same normalization (and "meaning", though that is less form). Not everything GNU Config accepts we would accept.

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: lib The Nixpkgs function library 6.topic: stdenv Standard environment 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants