lib.systems.parse: remove now-unnecessary windows hackery #249090
Conversation
Prior to this PR, gnu-config did its unpacking in the `buildPhase`,
which mean that `.override({ patches = ... })` couldn't be used to
test out patches to `gnu-config`.
This PR fixes that.
For almost a decade nixpkgs has parsed `*-*-{cygwin,msvc}` as being
ABIs for a common kernel (windows), rather than as kernels. It
isn't really reasonable to change nixpkgs' handling at this point,
so this commit replicates the special-case hack when unparsing, in
order to ensure that `(unparse . parse)==id`.
The eminent Donald E. Knuth should be recognized as having equal standing with such entities as IBM, Apple, and the Personal Computer. We should acknowledge this by including him as a "vendor". Also, `gnu-config` recognizes `mmix-knuth-*` triples (and in fact requires `vendor="knuth"` when `cpu="mmix"`) -- so we sort of have to. But we should do it anyways.
tripleFromSkeleton contains the same logic found in tripleFromSystem, but is capable of operating on unvalidated attrsets-of-strings.
gnu-config does not recognize `none` as a vendor -- that string describes a *kernel*
Unfortunately gnu-config triples are, in some rare cases, sensitive to whether the vendor was specified as `unknown` or was omitted. In other words, there are situations where these are not handled exactly the same way: - `mips-linux-gnu` - `mips-unknown-linux-gnu` This commit introduces `lib.systems.tripleFromSystemLossy`, which returns `"mips-unknown-linux-gnu"` for both of the platforms above. This is almost always what you want to use.
Since 4aa1ffa we have been shipping reverse-engineered out-of-tree forks of gcc and binutils for the Broadcom VC4. Upstream for those forks chose the nonstandard `vc4-elf` triple to identify their system. GNU config rejects this triple (and all `vc4-*` triples). This commit deals with the fallout from that. Mainly this commit ensures that we are consistent in using `vc4-elf` instead of `vc4-none`, and makes sure that parsing->unparsing round-trips properly.
GNU config (correctly) considers Solaris to be a BSD, and therefore appends its version to the kernel name, which should be `solaris2` not `solaris`. This commit updates `lib/systems` to do the same.
Each kernel supports only a subset of the universe of possible ABIs. This commit explicitly lists (opt-in) the allowable abis for each kernel. Attempting to parse a kernel with an abi which is not on the list will fail. This "search space pruning" is essential to getting our gnu-config agreement tests down to a manageable set of test cases. It also lets us reject nonsense triples instead of trying to handle them exactly the same way gnu-config does.
In PR #182807 we decided that big-endian PowerPC64 should default to the new elfv2 ABI. The implementation in that PR did the defaulting by adding two lines in the *triple parser*, which is turning out to be problematic; it means that our parse-then-unparse roundtrip disagrees with gnu-config. This commit therefore reverts just those two lines. If the defaulting logic needs to be moved elsewhere that should be done. https://github.com/NixOS/nixpkgs/pull/182807/files#r1234650738
This is required for agreement with gnu-config.
gnu-config doesn't include this.
This commit fixes the logic for handling missing/unknown vendor fields in triples, and extends it to handle mmix and microblaze CPU-types.
This commit adds lib/tests/triples.nix, which exhaustively tests that our platform triple parse-then-unparse round-trip agrees with gnu-config for all inputs that it accepts (it may -- and does -- reject triples which `gnu-config` accepts).
With this commit, we now parse or reject the following identically to gnu-config: - aarch64-solo5-none (accept) - aarch64-solo5-none-elf (reject) Closes #165836.
This commit adds support for triples with a missing kernel, rather than kernel="none". It also adds a parser case for triples which have a vendor and abi, but no kernel. This allows to parse "*-unknown-elf" triples. Closes #230160.
github-actions
bot
added
6.topic: rust
6.topic: lib
ghost
requested review from
… needed
Commit 3f32d4541dd4a08765a307363b368471c72d0348 bumped our
gnu-config to 2023-07-31, which includes
91f6a7f616b161c25ba2001861a40e662e18c4ad "config.sub: Accept
LLVM-style $cpu-$vendor-windows-{gnu,msvc}".
That commit allows us to remove some of the windows-triple hackery,
but the part that makes "cygnus" a valid ABI (note: this is
different from the part that makes "cygwin" a kernel!) cannot (yet)
be removed.
ghost
changed the title
ofborg
bot
added
10.rebuild-linux-stdenv
lib/systems/parse.nix
Outdated
| in | ||
| # gnu-config considers "mingw32" and "cygwin" to be kernels. | ||
| # This is obviously bogus, which is why nixpkgs has historically | ||
| # parsed them differently. However for regression testing | ||
| # reasons (see lib/tests/triples.nix) we need to replicate this | ||
| # quirk when unparsing in order to round-trip correctly. | ||
| if abi == abis.cygnus then "${cpu.name}-${vendor.name}-cygwin" | ||
| else if kernel == kernels.windows then "${cpu.name}-${vendor.name}-mingw32" | ||
| else "${cpu.name}-${vendor.name}-${kernelName kernel}${optExecFormat}${optAbi}"; |
There was a problem hiding this comment.
I think this should be possible to eliminate. You changed the normal form so the config we used in in examples.nix is in normal form. I would like to instead start migrating us to use ...-windows-gnu for the config in examples, and keep ...-windows-gnu as the normal form.
There was a problem hiding this comment.
Does your testing current use examples.nix? I think we can just skip the ...-{mingw32,cygwin} test cases but to test ...-windows-gnu and ...-windows-msvc to be able to land something before the migration of examples.nix is complete.
There was a problem hiding this comment.
I think this should be possible to eliminate.
It's not. Your patch to GNU config made msvc and gnu valid windows ABIs, but did not make cygnus a valid ABI. So we either have to keep doing the parse-time translation or else make a breaking change to anybody out-of-tree who's referencing lib.systems.abis.cygnus. I don't have any problem with a deprecation+migration for abis.cygnus, but that doesn't belong in this PR (a rebase of #235230 "add the test suite and make it pass").
You changed the normal form so the config we used in in
examples.nix
Does your testing current use
examples.nix?
No. This PR has nothing to do with examples.nix.
The test suite tests the parser (lib.systems.parser.mkSystemFromSkeleton). The parser is used by lots of things. Most things that use examples.nix (like pkgsCross) also use the parser; that's the only connection between the two.
There was a problem hiding this comment.
llvm/llvm-project@edbdd2e Ah I see, yes I only did 2 of these.
There was a problem hiding this comment.
I submitted a match to get cygnus too. It should appear in the mailing list archive too.
There was a problem hiding this comment.
So concretely, what I am proposing is that we remove this change, and instead filter out windows-cygnus from the generated test cases until the patch is merged. That would also keep things passing, right?
There was a problem hiding this comment.
https://lists.gnu.org/archive/html/config-patches/2023-08/msg00011.html here is the patch.
| # start with the lists of known possibilities for each field | ||
| (_: with lib.systems.parse; { | ||
| cpu = cpuTypes; | ||
| vendor = vendors; | ||
| kernel = kernels; | ||
| abi = abis; | ||
| }) |
There was a problem hiding this comment.
@Ericson2314 here is where the tests exhaustively enumerate the space of triples to be tested. examples.nix is not involved in any way, shape, or form. Note that since "missing" is a valid coordinate value for vendor, kernel, and abi this also enumerates non-canonical triples so they too get tested. It tests not only parsing but also canonicalization.
This test suite won't stop you from putting totally bogus triples into examples.nix. But it does stop the parser from accepting those bogus triples, so you won't be able to use pkgsCross.bogus-entry for anything without getting an error message.
I'd really like to get rid of these magic-lists-of-systems -- not only examples.nix but also doubles.nix which is approximately just as evil. This PR was carefully crafted to avoid using any of those, so when the day comes that we can remove them nothing here breaks.
There was a problem hiding this comment.
OK yes I agree this is better than going off examples.nix!
There was a problem hiding this comment.
OK yes I agree this is better than going off
examples.nix!
Which is now ready for deprecation and removal:
In 91f6a7f I supported `windows-gnu` and `windows-msvc`, but I forgot about the last one: `windows-cygnus`. This LLVM commit [1] introduced all 3. (I wish I had found it before!) But at least we can use it to ensure I am not missing one now. This came up in this Nixpkgs PR [2] where I was told my previous patch only partially solved the problem, because I forgot about this case. [1]: llvm/llvm-project@edbdd2e [2]: NixOS/nixpkgs#249090
ghost
mentioned this pull request
ghost
closed this
Description of changes
This is
rebased onto and updated for
Other than the rebase, the only changes relative to the above PR are in the last commit of this PR.
Surprisingly little of the cygwin/mingw hackery was able to be eliminated due to gnu-config commit 91f6a7f616b161c25ba2001861a40e662e18c4ad. But some of it was.
This PR does not cause a mass-rebuild. It has its base branch set to
stagingsimply because #247325 hasn't merged back tomasteryet.I've also used #249088 to test this with @Ericson2314's proposed patch found here, and all the tests (unsurprisingly) still pass.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)