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

Add cabal-paths patch for ghc 9.2.x #184041

Closed

Conversation

sloane-shark
Copy link
Contributor

@sloane-shark sloane-shark commented Jul 30, 2022

Description of changes

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+.

Tested by building and running haskell.packages.ghc923.ghcid.bin and haskell.packages.ghc923.ormolu.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@sloane-shark
Copy link
Contributor Author

nixpkgs-review pr 184041 failed on the jacinda package, but i tested on master and it fails for the same reason. seems like this should be marked as broken on (at least) aarch64-darwin. would it be worth including that in this PR, or not since it's unrelated?

@sloane-shark
Copy link
Contributor Author

Result of nixpkgs-review pr 184041 run on aarch64-darwin 1

2 packages built:
  • haskell.compiler.ghc923
  • haskell.compiler.native-bignum.ghc923

@sternenseemann
Copy link
Member

nixpkgs-review pr 184041 failed on the jacinda package, but i tested on master and it fails for the same reason. seems like this should be marked as broken on (at least) aarch64-darwin. would it be worth including that in this PR, or not since it's unrelated?

Feel free to add a conditional markBroken for regex-rure (which is actually failing) in this PR. Be sure to link the actual issue in a comment as well: haskell/c2hs#33 (comment)

@sloane-shark
Copy link
Contributor Author

sloane-shark commented Jul 31, 2022

@sternenseemann Any recommendation for a cleaner method of doing this?

{
  # ...
  regex-rure = doDistribute ((if isDarwin && isAarch64 then x: x else markUnbroken) super.regex-rure);
  # ...
}

Seems to work fine, just looks a little confusing imo

@sternenseemann
Copy link
Member

Any recommendation for a cleaner method of doing this?

There's an override section in configuration-darwin.nix that only gets applied on aarch64-darwin (towards the bottom of the file).

@sloane-shark
Copy link
Contributor Author

This patch didn't seem to work correctly, trying to build .#jacinda failed with the original error instead of reporting regex-rure broken. Is the GHC configuration overriding the darwin configuration?

@sternenseemann
Copy link
Member

Don't remember the ordering precisely (you can check pkgs/development/haskell-modules/default.nix) in any case, you don't have to have any override in the regular configuration, if your patch only touches configuration-darwin.nix, it should wrok.

@sloane-shark
Copy link
Contributor Author

sloane-shark commented Aug 3, 2022

I can't work out exactly what's going on, but it seems like ...

  • regex-rure is generally marked as broken in hackage-packages.nix
  • configuration-darwin.nix "re"marks it as broken, which is ignored
  • configuration-9.2.x.nix marks it as unbroken, which is the final accepted setting

I can update the PR with this commit so someone can maybe take a look and see if I am missing something. Otherwise I could

  • leave the PR as-is, which correctly marks the package broken on aarch64-darwin
  • drop the broken-marking entirely, and leave that for another PR

edit: i realize i am somewhat assuming that the package will build properly on linux, but is that the case? Is gcc used for C2hs on linux? if not, maybe i could remove the override in configuration-9.2.x.nix to resolve this.

@sloane-shark
Copy link
Contributor Author

@sternenseemann sorry for the ping, i've pushed a version that i can get to locally report regex-rure as broken, i was unable to get something working via the configuration-9.2.x.nix file. if it's better, i can just drop the version specific hacking and leave the broken packages as-is for now. i think this should be ready for review now, would it be fair to ping anyone? otherwise i can hold tight until someone has the bandwidth.

@sternenseemann
Copy link
Member

regex-rure is generally marked as broken in hackage-packages.nix

Ah, right I remember now. The problem is that regex-rure only builds with GHC 9.2 and above, so you can only expect haskell.packages.ghc924.regex-rure to work and it is marked as broken by default (since that follows the state in the default package set). Leaving the override as is is okay (albeit a bit ugly), there doesn't seem to be a better solution due to the overlay chaining.

@sternenseemann
Copy link
Member

Can you rebase on master? I think the regex-rure hack can be dropped now. The patch will need to be applied to 9.2.4 and 9.2.5 now.

Porting that stuff to 9.4.* can be done in a separate step then.

@srid
Copy link
Contributor

srid commented Jan 3, 2023

This patch should be applied for 9.0 (pkgs.haskellPackages) as well, no? Because of #194367 (comment)

@sternenseemann
Copy link
Member

This patch should be applied for 9.0 (pkgs.haskellPackages) as well, no? Because of #194367 (comment)

haskellPackages is 9.2 now, 9.0 had and has a patch for this already which no longer applies for Cabal 3.8 shipped with GHC 9.2.

@sloane-shark sloane-shark changed the title Add cabal-paths patch for ghc 9.2.3 Add cabal-paths patch for ghc 9.2.x Jan 4, 2023
@sloane-shark
Copy link
Contributor Author

sloane-shark commented Jan 4, 2023

Looks like the patch that got added to all builds for 9.2.x (https://github.com/haskell/cabal/commit/6c796218c92f93c95e94d5ec2d077f6956f68e98.patch) updates some of the same files as this patch, so I have to hardcode it to address those changes. I would say this should be fine, except for the note that says

Can be removed if the Cabal library included with ghc backports the linked fix

But I guess that could just be addressed with a revert of the appropriate commit?

@sloane-shark
Copy link
Contributor Author

I'm also still seeing the regex-rure failure show up in local testing, for what it's worth.

@sloane-shark sloane-shark force-pushed the fix/ghc923-cabal-paths-patch branch 3 times, most recently from 097e631 to 12a98a8 Compare January 4, 2023 23:26
@sloane-shark
Copy link
Contributor Author

alright, i think this should be ready for another pass of reviewing. i can kick off nixpkgs-review, but as per comments above, is there anything i can do to more thoroughly verify things are working as expected? i can try running ghcid or ormolu as I did in my initial post

@flokli
Copy link
Contributor

flokli commented Feb 17, 2023

This mass-subscribed a bunch of people, who won't get unsubscribed from retrieving notifications.

Please open a new PR.

@flokli flokli closed this Feb 17, 2023
@NixOS NixOS locked and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

5 participants