Skip to content

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

Merged
Ericson2314 merged 1 commit intomasterfrom
unknown repository
Nov 22, 2022
Merged

lib/systems/parse.nix: mkSkeletonFromList: improve readability#180964
Ericson2314 merged 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jul 10, 2022 •
edited by ghost
Loading

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 requested review from Ericson2314, alyssais, matthewbauer and nbp as code owners July 10, 2022 10:29
@ghost ghost marked this pull request as draft July 10, 2022 10:29
@ghost

This comment was marked as outdated.

Sign in to view
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jul 10, 2022
@ghost
Copy link
Copy Markdown
Author

ghost commented Jul 11, 2022 •
edited by ghost
Loading

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
Copy Markdown
Author

ghost commented Jul 11, 2022

@sternenseemann should be added as a reviewer here.

@ghost ghost mentioned this pull request Jul 27, 2022
Closed
13 tasks
@sternenseemann sternenseemann self-requested a review July 27, 2022 19:55
@ghost
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Ericson2314 commented Nov 4, 2022

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

Ericson2314
Ericson2314 reviewed Nov 4, 2022
View reviewed changes
@ghost ghost requested review from Ericson2314 and removed request for alyssais, matthewbauer, nbp and sternenseemann November 5, 2022 01:46
sternenseemann
sternenseemann approved these changes Nov 5, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Ericson2314
Ericson2314 reviewed Nov 5, 2022
View reviewed changes
@ghost
Copy link
Copy Markdown
Author

ghost commented Nov 14, 2022

Merge conflict due to #82131.

@ghost ghost marked this pull request as draft November 14, 2022 06:53
@Ericson2314
lib/systems/parse.nix: mkSkeletonFromList: improve readability
36a566b 
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
Copy Markdown
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
Copy Markdown
Member

Ericson2314 commented Nov 22, 2022

Sorry we didn't merge before! :(

@github-actions github-actions bot mentioned this pull request Nov 22, 2022
Merged
1 task
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 22, 2022

Successfully created backport PR #202405 for release-22.11.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 22, 2022

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

@ghost
Copy link
Copy Markdown
Author

ghost commented Nov 22, 2022 •
edited by ghost
Loading

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
Open
12 tasks
@Artturin Artturin mentioned this pull request Jan 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Ericson2314 @sternenseemann