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

haskellPackages.hail: add patches to relax cabal dependencies #72923

Merged
merged 1 commit into from Nov 7, 2019

Conversation

@astro
Copy link
Contributor

astro commented Nov 6, 2019

Motivation for this change

Didn't build.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @ehmry

@astro astro requested review from basvandijk and cdepillabout as code owners Nov 6, 2019
@ofborg ofborg bot added the 6.topic: haskell label Nov 6, 2019
@ehmry
ehmry approved these changes Nov 6, 2019
Copy link
Member

cdepillabout left a comment

@astro In order to get this merged, could you do the following things:

  1. For each patch, add a link to the PR it comes from and a short description of why it is needed (what it does).
  2. Ask upstream to make a new release of hail with these patches pulled in. Since these patches appear to only be dependency version fixes, it could just be a Hackage revision instead of a full release. If you added a comment to this PR explaining this, and saying which version of hail you expect to be working without applying these patches, it would be helpful for the nixpkgs maintainers to know when we can remove these patches.
  3. Remove this line: . You can find an explanation of this here: https://discourse.nixos.org/t/video-tutorial-how-to-fix-broken-haskell-packages-in-nix/3968.
  4. Change the base branch for this PR to haskell-updates instead of master. Then actually rebase the commit in this PR onto the haskell-updates branch.
@tfc

This comment has been minimized.

Copy link
Contributor

tfc commented Nov 7, 2019

The first patch is already merged to master, and the second patch is a PR that just needs to be merged. how about contacting @shlevy about this to have both patches in and then simply bumping src up to the latest commit on the master branch?

I would profit from this too, as we use hail in our intranet.

@astro astro force-pushed the astro:fix-hail branch from 3018251 to 0b67141 Nov 7, 2019
@astro

This comment has been minimized.

Copy link
Contributor Author

astro commented Nov 7, 2019

@cdepillabout Thank you for your writeup. I hope to have addressed all concerns.

@astro

This comment has been minimized.

Copy link
Contributor Author

astro commented Nov 7, 2019

The first patch is already merged to master, and the second patch is a PR that just needs to be merged. how about contacting @shlevy about this to have both patches in and then simply bumping src up to the latest commit on the master branch?

Ok, I have asked in the 2nd PR.

We still would love to have unbroken Hail in 19.09 rather soon than waiting for a single maintainer.

@@ -1292,4 +1292,22 @@ self: super: {
# https://github.com/Happstack/web-routes-th/pull/3
web-routes-th = doJailbreak super.web-routes-th;

# Remove for hail > 0.2.0.0

This comment has been minimized.

Copy link
@cdepillabout

cdepillabout Nov 7, 2019

Member

Comments here look good.

@@ -43,7 +43,7 @@ core-packages:
- ghcjs-base-0

default-package-overrides:
# LTS Haskell 14.12
# LTS Haskell 14.13

This comment has been minimized.

Copy link
@cdepillabout

cdepillabout Nov 7, 2019

Member

You don't actually need to regenerate the default-package-overrides part of this file.

In general, @peti will do this once a week a or so.

@@ -5353,7 +5263,6 @@ broken-packages:
- Haggressive
- hahp
- haiji
- hail

This comment has been minimized.

Copy link
@cdepillabout

cdepillabout Nov 7, 2019

Member

All you need to do is remove this line from the broken-packages section.

This comment has been minimized.

Copy link
@astro

astro Nov 7, 2019

Author Contributor

Oops, the rest slipped in due to a last test build.

This comment has been minimized.

Copy link
@astro

astro Nov 7, 2019

Author Contributor

Actually, no. I need to update the PR's target branch.

@@ -11942,21 +11942,22 @@ self: {

"LDAPv3" = callPackage
({ mkDerivation, base, base-encoding, binary, bytestring
, containers, deepseq, int-cast, newtype, quickcheck-instances

This comment has been minimized.

Copy link
@cdepillabout

cdepillabout Nov 7, 2019

Member

This file shouldn't be modified either, since @peti does it once a week or so.

@astro astro changed the base branch from master to haskell-updates Nov 7, 2019
@cdepillabout

This comment has been minimized.

Copy link
Member

cdepillabout commented Nov 7, 2019

@astro Thanks for updating this.

However, you'll need to do the following things before this can be merged in:

  1. Change the base branch for this PR to haskell-updates instead of master. Then actually rebase the commit in this PR onto the haskell-updates branch.
  2. Make sure not to modify the pkgs/development/haskell-modules/hackage-packages.nix file, since this is done manually once-a-week or so by @peti.
  3. Make sure not to regenerate the default-package-overrides section of the pkgs/development/haskell-modules/configuration-hackage2nix.yaml file, since this is also done by @peti.

We still would love to have unbroken Hail in 19.09 rather soon than waiting for a single maintainer.

As soon as the above are fixed, I think this should be fine to be merged in. Although when a new release of hail is made, we'd be very grateful if you could send another PR removing the patches added in this PR. (I imagine the patches will fail to apply, hail will fail to compile, and it might be marked broken again.)

@cdepillabout

This comment has been minimized.

Copy link
Member

cdepillabout commented Nov 7, 2019

@astro Okay, it looks like you fixed everything :-)

I'll merge after CI passes. I've confirmed this builds locally.

@cdepillabout

This comment has been minimized.

Copy link
Member

cdepillabout commented Nov 7, 2019

Okay, CI has passed, so merging in.

@astro Thanks again. This should be available when the haskell package set has been regenerated (usually in a week or so).

@cdepillabout cdepillabout merged commit 97982e0 into NixOS:haskell-updates Nov 7, 2019
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@astro astro deleted the astro:fix-hail branch Nov 8, 2019
@shlevy

This comment has been minimized.

Copy link
Member

shlevy commented Nov 8, 2019

Will upload a fixed version of hail to hackage, sorry!

@cdepillabout cdepillabout mentioned this pull request Nov 11, 2019
1 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.