Add cabal-paths patch for ghc 9.2.x #216857
Conversation
sloane-shark
requested review from
cdepillabout,
sternenseemann and
maralorn
as code owners
sloane-shark
force-pushed
the
fix/ghc923-cabal-paths-patch
branch
from
80218f1
to
22b690e
Compare
There was a problem hiding this comment.
Changes had been requested on the last version of this PR, #184041
| + | (splitPath flat_prefix) `isPrefixOf` (splitPath flat_bindir) = True | ||
| + | (splitPath flat_prefix) `isPrefixOf` (splitPath p) = False |
There was a problem hiding this comment.
@sternenseemann Continuing the review from the last PR, what do you think is the proper fix here? I can't work out why splitPath was included originally (I copied this from the existing cabal-paths.patch file), but I imagine the correct code would be one of the following:
| + | (splitPath flat_prefix) `isPrefixOf` (splitPath flat_bindir) = True | |
| + | (splitPath flat_prefix) `isPrefixOf` (splitPath p) = False | |
| + | flat_prefix `isPrefixOf` flat_bindir = True | |
| + | flat_prefix `isPrefixOf` p = False |
| + | (splitPath flat_prefix) `isPrefixOf` (splitPath flat_bindir) = True | |
| + | (splitPath flat_prefix) `isPrefixOf` (splitPath p) = False | |
| + | [flat_prefix] `isPrefixOf` (splitPath flat_bindir) = True | |
| + | [flat_prefix] `isPrefixOf` (splitPath p) = False |
There was a problem hiding this comment.
Tested both of these with ghcid, and the top one seems to work as expected, while the bottom one fails to resolve the "cycle detected" error. I will go ahead and rework it.
There was a problem hiding this comment.
Resolved this with a variation on the top patch, which gets rid of splitPath entirely.
ofborg
bot
added
the
2.status: merge conflict
sloane-shark
force-pushed
the
fix/ghc923-cabal-paths-patch
branch
from
22b690e
to
d248aa8
Compare
|
Confirmed that
I've tried |
ofborg
bot
removed
the
2.status: merge conflict
ofborg
bot
added
10.rebuild-darwin: 0
sloane-shark
changed the title
sloane-shark
force-pushed
the
fix/ghc923-cabal-paths-patch
branch
from
c9dce0e
to
ca08298
Compare
|
Result of 55 packages marked as broken and skipped:
19 packages failed to build:
179 packages built:
|
|
@srid @domenkozar if either of you has an x86_64-darwin machine to test this PR, I think that would help make it a bit more convincing for merging. maybe for reference, when I try to run ❯ nix run .#haskell.packages.ghc924.ghcid.bin
Loading ghci -fno-code -fno-break-on-exception -fno-break-on-error -v1 -ferror-spans -j ...
GHCi, version 9.2.4: https://www.haskell.org/ghc/ :? for help
No files loaded, GHCi is not working properly.
Command: ghci -fno-code -fno-break-on-exception -fno-break-on-error -v1 -ferror-spans -jcan't tell if this is due to the patch, or some unrelated issue. |
|
another strange error for ormolu: ❯ nix run .#haskell.packages.ghc924.ormolu
error: unable to execute '/nix/store/1y4bg6lk0qqlrpqhnxw1j83bgw5c2rds-ormolu-0.5.0.1/bin/ormolu': No such file or directory
❯ ls /nix/store/1y4bg6lk0qqlrpqhnxw1j83bgw5c2rds-ormolu-0.5.0.1/
lib nix-support |
|
I think we can actually make it even simple, I'll try to come up with an alternative patch and a way to test this that doesn't require rebuilding GHC everytime which will probably come in handy. |
|
@matthewess What do you think of this? I tried moving all code generation logic into the Template, so that only the decision whether stuff is emitted or not (and the generation of the warning text) is left in the For testing I have the following Nix expression which uses my local working copy of { pkgs ? import <nixpkgs> { } }:
let
inherit (pkgs) haskellPackages lib;
Cabal = haskellPackages.callCabal2nix "Cabal.cabal.nix" ~/src/hs/ghc/libraries/Cabal/Cabal { };
in
haskellPackages.mkDerivation {
pname = "paths-test";
version = "0";
src = ./.;
setupHaskellDepends = [ Cabal ];
license = lib.licenses.bsd3;
# Change these to make the patch do something
enableSeparateDataOutput = false;
enableSeparateBinOutput = false;
} |
|
@sternenseemann that looks very clean! i can try and work those changes into this PR this weekend |
|
@matthewess If you want, I can take that off your hands; then I'd just need your help with reviewing and testing (since I lack the hardware). |
sternenseemann
force-pushed
the
fix/ghc923-cabal-paths-patch
branch
from
d3bf60e
to
d1c3fa9
Compare
|
@ofborg build haskell.compiler.ghc927 haskell.compiler.ghc926 For anyone reviewing, viewing the patch this way may be easier. |
sternenseemann
force-pushed
the
fix/ghc923-cabal-paths-patch
branch
from
d1c3fa9
to
f26a669
Compare
|
@ofborg build haskell.compiler.ghc927 haskell.compiler.ghc926 |
|
What's the current situation on this PR? This is a pretty major blocker for many users. It seems that ofborg build failed with a timeout? |
|
Needs to be tested still. I think it's okay, but would be preferable if someone else could also review the actual code. |
sternenseemann
force-pushed
the
fix/ghc923-cabal-paths-patch
branch
from
f26a669
to
77764d3
Compare
|
Playing around with this today. Are there any good test candidates besides making sure that e.g. |
|
@matthewess plus |
There was a problem hiding this comment.
I cannot claim that I have understood this change in full depth. Especially since I hadn’t looked at the Cabal code base before.
Still, this change is well documented, and the code looks like it does what the comment says. Also, to prove that I really read it, I found a subtle typo.
pkgs/development/compilers/ghc/Cabal-3.6-paths-fix-cycle-aarch64-darwin.patch
Outdated
Show resolved
Hide resolved
|
The following commands worked for me on ❯ nix run .#haskell.packages.ghc924.ormolu.bin
❯ nix run .#haskell.packages.ghc924.ghcid.bin -- --help
❯ nix run .#haskell.packages.ghc924.niv.bin
❯ nix run .#haskell.packages.ghc924.citeproc.bin -- --helpattempting to run |
|
@matthewess The thing for ormolu is to be expected as you were not actually looking at the |
Due to link time dead code elimination not working on aarch64-darwin, some unused store path references in Paths_* modules are retained. This causes reference cycles when a separate `bin` output is used. To prevent this, add a patch to Cabal as shipped by GHC which infers based on the installation layout (which is influenced by enableSeparateBinOutput, enableSeparateDataOutput etc. in a Nix build) which references can be retained without causing a reference cycle. This ensures that packages that were fine with a bin output will also work on aarch64-darwin. Packages that cause a reference cycle anyways (by actually using references that do cause one) fail due to a missing symbol – here we are trading the overall benefit for a more confusing error message. For details, refer to the explanation comment in the patch.
sternenseemann
force-pushed
the
fix/ghc923-cabal-paths-patch
branch
from
77764d3
to
34e7e34
Compare
Good catch, you can see I figured that out for ormolu but I messed up with ❯ nix run .#haskell.packages.ghc924.nfc.bin
error: unable to execute '/nix/store/pd62xcxz72g335axy6872002fihq9qp1-nfc-0.1.1-bin/bin/nfc': No such file or directory
❯ ls /nix/store/pd62xcxz72g335axy6872002fihq9qp1-nfc-0.1.1-bin/bin/
print-mifare-uid-forever |
|
@matthewess So GHC is still working on aarch64-darwin? Should we merge? |
Yes, GHC is working! My somewhat informed opinion on merging is also yes. I'm no Haskell maintainer, but this seems to fix the symptom that prompted the original PR well, and your changes set us up better to adjust it in the future if something ends up being amiss. |
This ports our infamous patch for `Cabal` which cheesily prevents an output cycle for derivations that use separate bin outputs where references caused by the `Paths_*` module can't be eliminated by the GHC aarch64-darwin codegen backend. See also - the original issue NixOS#140774, - the original patch for GHC 9.2 NixOS#216857 - the ported patch for GHC 9.4 NixOS@f6f780f Co-authored-by: sternenseemann <sternenseemann@systemli.org>
This ports our infamous patch for `Cabal` which cheesily prevents an output cycle for derivations that use separate bin outputs where references caused by the `Paths_*` module can't be eliminated by the GHC aarch64-darwin codegen backend. See also - the original issue #140774, - the original patch for GHC 9.2 #216857 - the ported patch for GHC 9.4 f6f780f Co-authored-by: sternenseemann <sternenseemann@systemli.org>
This ports our infamous patch for Cabal that prevents certain parts of the Paths_* module from being generated in order to prevent unnecessary references on aarch64-darwin, to GHC >= 9.10. See also: - Original issues: #140774 - Patches - Original patch for GHC >= 8.10 && < 9.2 / Cabal >= 3.2 && < 3.6: b0dcd7f - Patch for GHC >= 9.2 && < 9.10 / Cabal >= 3.6 && < 3.12: #216857, f6f780f129f50df536fb30, …
This ports our infamous patch for Cabal that prevents certain parts of the Paths_* module from being generated in order to prevent unnecessary references on aarch64-darwin, to GHC >= 9.10. See also: - Original issues: #140774 - Patches - Original patch for GHC >= 8.10 && < 9.2 / Cabal >= 3.2 && < 3.6: b0dcd7f - Patch for GHC >= 9.2 && < 9.10 / Cabal >= 3.6 && < 3.12: #216857, f6f780f129f50df536fb30, … (cherry picked from commit 3885dfe)
This ports our infamous patch for Cabal that prevents certain parts of the Paths_* module from being generated in order to prevent unnecessary references on aarch64-darwin, to GHC >= 9.10. See also: - Original issues: #140774 - Patches - Original patch for GHC >= 8.10 && < 9.2 / Cabal >= 3.2 && < 3.6: b0dcd7f - Patch for GHC >= 9.2 && < 9.10 / Cabal >= 3.6 && < 3.12: #216857, f6f780f129f50df536fb30, … (cherry picked from commit 3885dfe)
Description of changes
Continuation of #184041
I tweaked the cabal-paths.patch file included for GHC builds (see #140774) to work on the updated Cabal PathsModule code that is used in GHC 9.2.3+.
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/)