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/parse.nix: mkSkeletonFromList: improve readability #180964

Merged
merged 1 commit into from Nov 22, 2022
Merged

lib/systems/parse.nix: mkSkeletonFromList: improve readability #180964

merged 1 commit into from Nov 22, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jul 10, 2022

Motivation

The main purpose of this PR is to make the basis for mkSkeletonFromList's decision between cpu-kernel-libcabi vs cpu-vendor-os clear, without changing its behavior. The existing code obscures this decision behind a sequence of prioritized matches (i.e. if-then) which jump around between different coordinates.

Two side benefits of this PR:

  1. It makes the root cause of Embedded target triplets (system, config) are used and interpreted incorrectly #165836 obvious: we are missing a case for cpu-vendor-libcabi. This is why nixpkgs stumbles over *-none-*.

  2. It illuminates some very weird corner cases in the existing logic, like *-${vendor}-ghcjs overriding the vendor field, and mingw32 being transformed into windows in some cases.

Instructions for verifying no behavior change

curl -L https://raw.githubusercontent.com/amjoseph-nixpkgs/nixpkgs/a9595374806477c40eab88acbc06ca7d641364ea/lib/systems/test.nix > lib/systems/test.nix
nix eval --raw -f lib/systems/test.nix | egrep -ve '-apple-(mingw32|eabi|elf|ghcjs)' > after
git switch --detach origin/master
nix eval --raw -f lib/systems/test.nix | egrep -ve '-apple-(mingw32|eabi|elf|ghcjs)' > before
diff -u before after

The diff should produce no output. You should verify that the *-apple-* tuples removed by the egrep -v filters are nonsensical (changing the behavior in these nonsensical cases allowed simplification -- see below for details). Here is how lib/systems/test.nix was created:

  1. Copy-and-paste of the old (unsimplified) version of mkSkeletonFromList
  2. Comment it out
  3. Form a list consisting of one or more attrsets for each possible coordinate value which could participate in triggering each of the if clauses in the original code
  4. Append "default values" for each of the three coordinates.
  5. Run mkSkeletonFromList on the cartesianProductOfSets of the attrsets.

Note that if vendor is missing from the return value of mkSkeletonFromList, lib/systems/test.nix will set vendor="unknown"

Things done
  • Built on platform(s)
    • x86_64-linux
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@ghost ghost marked this pull request as draft July 10, 2022 10:29
@ghost

This comment was marked as outdated.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jul 10, 2022
@ghost
Copy link
Author

ghost commented Jul 11, 2022

Good to go. For ease of reviewing, see the copypasta at the top of the first post.

Please squash before merging, or ask me to do so.

@ghost ghost marked this pull request as ready for review July 11, 2022 05:17
@ghost
Copy link
Author

ghost commented Jul 11, 2022

@sternenseemann should be added as a reviewer here.

@ghost
Copy link
Author

ghost commented Sep 4, 2022

@sternenseemann and @Ericson2314, have you had a chance to look at this yet?

There are a few PRs open (like #186484) which, if merged ahead of this one, will invalidate the proof that the behavior of mkSystemFromString is unchanged.

@ghost
Copy link
Author

ghost commented Sep 17, 2022

Ping @sternenseemann @Ericson2314 @alyssais @nbp

I want to submit other changes which will create a merge conflict with this PR. What should I do?

We're starting to run into a lot more problems with calling Musl an ABI. Nixpkgs needs understand that "big-endian 64-bit powerpc on Musl always uses the ELFv2 ABI (see #182807)", but since nixpkgs thinks that Musl is an ABI there is no way to express this other than special-purpose hackery like #191436. The fact that {build,host,target}Platform don't have separate fields for libc and abi which can be matched independently is the main obstacle. Fixing that will create a merge conflict with this PR.

I've radically shortened the description at the top of this PR in the hope of attracting some kind of review.

@Ericson2314
Copy link
Member

@amjoseph-nixpkgs this looks good! Sorry I didn't see it for so long, Please harass me on Matrix in #cross-compiling:nixos.org to make sure this stuff gets reviewed, this is not the first time you made a good thing that went neglected by us!

lib/systems/parse.nix Outdated Show resolved Hide resolved
@ghost ghost requested review from Ericson2314 and removed request for matthewbauer, nbp, alyssais and sternenseemann November 5, 2022 01:46
Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

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

Also did some half-elaborate sanity checking of my own which seems to check out.

One thing is that some things in parse.nix have changed since July, but I'm not sure if we need to adjust any code in here necessarily.

lib/systems/parse.nix Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Nov 14, 2022

Merge conflict due to #82131.

@ghost ghost marked this pull request as draft November 14, 2022 06:53
The main purpose of this PR is to make the basis for
`mkSkeletonFromList`'s decision between `cpu-kernel-libcabi` vs
`cpu-vendor-os` clear, without changing its behavior.  The existing
code obscures this decision behind a sequence of prioritized matches
(i.e. `if-then`) which jump around between different coordinates.

Two side benefits of this PR:

1. It makes the root cause of #165836 obvious: we are missing a case
   for `cpu-vendor-libcabi`.  This is why nixpkgs stumbles over
   `*-none-*`.

2. It illuminates some very weird corner cases in the existing
   logic, like `*-${vendor}-ghcjs` overriding the `vendor` field,
   and `mingw32` being transformed into `windows` in some cases.

Co-authored-by: John Ericson <git@JohnEricson.me>
@ghost
Copy link
Author

ghost commented Nov 14, 2022

Squashed, because it wasn't worth rebasing past 25 separate commits.

I've added *-*-freebsd* to the cpu-vendor-os fork of the *-*-* case.

Note that freebsd can be used in the other (cpu-kernel-environment) fork as well; x86_64-freebsd-gnu is still valid although I don't think glibc still builds on freebsd. But there's no need to parse it unless somebody is serious about adding that capability.

@ghost ghost marked this pull request as ready for review November 14, 2022 07:30
@Ericson2314 Ericson2314 merged commit 2cb8f1a into NixOS:master Nov 22, 2022
@Ericson2314
Copy link
Member

Sorry we didn't merge before! :(

@github-actions
Copy link
Contributor

Successfully created backport PR #202405 for release-22.11.

@github-actions
Copy link
Contributor

Git push to origin failed for release-22.11 with exitcode 1

@ghost
Copy link
Author

ghost commented Nov 22, 2022

Git push to origin failed for release-22.11 with exitcode 1

I think sterni fixed this in #202405. If not, or if I need to do anything more, ping me.

@ghost ghost deleted the pr/mkSkeletonFromList/simplify branch November 22, 2022 23:52
@ghost ghost mentioned this pull request May 7, 2023
12 tasks
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants