lib/systems/inspect.nix: replace isPowerPC with isPower32BigEndian#168113
lib/systems/inspect.nix: replace isPowerPC with isPower32BigEndian#168113sternenseemann merged 1 commit intomasterfrom unknown repository
Conversation
sternenseemann
left a comment
There was a problem hiding this comment.
GHC supports PowerPC in general (see this wiki page), so to match the intention, the predicate should be changed to isPower everywhere GHC-related, I believe.
I suspect this is the case in other places as well, as it is indeed confusingly named, good that you caught that!
|
That's actually what I'm working on next...
Sorry, I cut-and-pasted that line from an unsubmitted earlier version of this PR by mistake. I have corrected my comment at the top of the thread. That line currently reads:
Sorry, I don't quite understand? This PR changes the predicate from |
|
Squashed @sternenseemann's changes into a single commit and fixed the typo in the commit message. No other changes. |
This comment was marked as resolved.
This comment was marked as resolved.
|
There is an official bindist for GHC 8.6.5 for I don't think nixpkgs can cross compile GHC at the moment (it can build a GHC cross compiler), GHC's cross build system is probably going to give you a lot of trouble in that case. |
This comment was marked as resolved.
This comment was marked as resolved.
|
@ofborg eval (ofborg seems to have become stuck) |
@sternenseemann sorry, I still do not understand what you are asking for. I must be really stupid. After this commit, there are no occurrences of the string If this has not resolved your request, could you please make a self-contained statement of what you are asking me to do? |
|
@sternenseemann in this comment, were you suggesting that the following substitutions be made (on top of this PR)? I am fine with that. I originally picked On the other hand, the autoconf/gcc multiarch tuple for this architecture is So really it doesn't matter to me. If you were asking for the substitutions above just say so and I will implement them (and also update #168111 which I pushed for a merge of because I needed to use that predicate in other PRs). At this point it's still very easy to make changes like this, and I don't have a strong preference one way or the other. |
sternenseemann
left a comment
There was a problem hiding this comment.
Sorry, I hope I'll manage to clear up the misunderstanding this time!
My request was not to change how any predicate is named, but which predicate is used in certain places.
For GHC I can say it with some confidence: Where the predicate is used we decide whether the GHC -fasm backend is available or we have to use LLVM. From looking at the code, GHC's Power(PC) backend supports both 32 and 64 bits as well as both endianess settings. Therefore we should check for the CPU family instead of a specific subarchitecture in that place (see the suggestions).
(My suspicion is that it is similar for other uses you touched in this PR, but I guess we shouldn't change anything without checking probably which I haven't so far.)
|
Latest push applies all of @sternenseemann's suggestions and squashes to a single commit. Thank you so much for the clarification! Makes perfect sense now. |
There was a problem hiding this comment.
I checked and the xz man page says it supports
PowerPC 4 Big endian only
While it doesn't explicitly say 64bit is not supported, it seems that way. So this seems correct after all!
There was a problem hiding this comment.
This is a bit bizarre and I have no clue what this is supposed to be doing, so let's not touch it. Interestingly the only dir this could be referring to is sound/ppc which is a bit confusing to me.
@Ericson2314 extraIncludeDirs is only used here in nixpkgs, maybe we can remove it?
|
I think this deserves a brief changelog entry (under “Backwards incompatible changes”) and then we should be good to go! |
Done. I noticed that after your changes, there were only three uses of this predicate remaining. How about we just inline its definition and remove the predicate entirely? Since both renaming it and deleting it are both backwards-incompatible changes, I further suggest doing both at the same time so we only have one such change rather than two. I've pushed an extra commit onto this PR which does what is described in the previous paragraph. If that's not okay, you can merge all-but-the-last commit in this series. Let me know if I should squash all or some of the commits. |
|
Okay, github caught up with itself. Currently waiting for |
Finally finished. It tried to remove the |
sternenseemann
left a comment
There was a problem hiding this comment.
Found some typose we missed previously.
|
Is it important to you that this makes 22.05? We can still backport this to the release branch until the actual release is cut. I'm not sure, I guess it depends on whether you plan on backporting other fixes for powerpc64le-linux later as well. |
No, it is not -- but thanks for asking. If the nixpkgs-genie gave me one wish, it would be #168413 so we can finally commit a Also, I know I need to update the manual docbook for this PR. I'm still waiting for pandoc to rebuild. Will do so as soon as it finishes building. |
Okay, that also gives downstream more time to find out this platform attribute is missing – as I've discovered there's no good mechanism for removing these attributes with a good error message for downstream users. |
Very confusingly, the `isPowerPC` predicate in
`lib/systems/inspect.nix` does *not* match `powerpc64le`!
This is because `isPowerPC` is defined as
isPowerPC = { cpu = cpuTypes.powerpc; };
Where `cpuTypes.powerpc` is:
{ bits = 32; significantByte = bigEndian; family = "power"; };
This means that the `isPowerPC` predicate actually only matches the
subset of machines marketed under this name which happen to be 32-bit
and running in big-endian mode which is equivalent to:
with stdenv.hostPlatform; isPower && isBigEndian && is32bit
This seems like a sharp edge that people could easily cut themselves
on. In fact, that has already happened: in
`linux/kernel/common-config.nix` there is a test which will always
fail:
(stdenv.hostPlatform.isPowerPC && stdenv.hostPlatform.is64bit)
A more subtle case of the strict isPowerPC being used instead of the
moreg general isPower accidentally are the GHC expressions:
Update pkgs/development/compilers/ghc/8.10.7.nix
Update pkgs/development/compilers/ghc/8.8.4.nix
Update pkgs/development/compilers/ghc/9.2.2.nix
Update pkgs/development/compilers/ghc/9.0.2.nix
Update pkgs/development/compilers/ghc/head.nix
Since the remaining legitimate use sites of isPowerPC are so few, remove
the isPowerPC predicate completely. The alternative expression above is
noted in the release notes as an alternative.
Co-authored-by: sternenseemann <sternenseemann@systemli.org>
|
I've squashed the branch and moved the Changelog entry to the 21.11 release notes. Let me know if you're happy with the commit message and I'll merge. |
Thank you! Looks good to me. |
Description of changes
Very confusingly, the
isPowerPCpredicate inlib/systems/inspect.nixdoes not matchpowerpc64le!This is because
isPowerPCis defined asWhere
cpuTypes.powerpcis:This means that the
isPowerPCpredicate actually only matches the subset of machines marketed under this name which happen to be 32-bit and running in big-endian mode.This seems like a sharp edge that people could easily cut themselves on. In fact, that has already happened: in
linux/kernel/common-config.nixthere is a test which will alwaysfail:
Let's rename the predicate to
isPower32BigEndiansince that's what it really checks. This predicate is used in only three places: ghc, linux/kernel, and nixos'siso-image.nix.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/)nixos/doc/manual/md-to-db.shto update generated release notes