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

hasura: 1.2.1 -> 1.3.1-beta.1 #95369

Merged
merged 1 commit into from Sep 3, 2020

Conversation

ingenieroariel
Copy link
Member

@ingenieroariel ingenieroariel commented Aug 13, 2020

Motivation for this change

This upgrades Hasura to use GHC 8.10 in anticipation of the 20.09 release. I was pleasantly surprised when I found it available on 20.03 and sad when it went missing from nixos-unstable.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • [x ] 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 nixpkgs-review --run "nixpkgs-review wip"
  • [ x ] 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.

@ingenieroariel
Copy link
Member Author

Ping @offlinehacker @peti

@ingenieroariel
Copy link
Member Author

It was very hard to track this with GHC86 so I just looked for the first version that supported GHC810 and was able to get it to work with some small mods.

There is a problem with infinite recursion when trying to override some packages like time, so I agree with @peti 's analysis that this recipe is brittle, but on the other hand, since it is an application and not a library I'd say it is okay to pin some of its dependencies.

@ofborg ofborg bot requested a review from kalbasit August 14, 2020 00:31
Copy link
Member

@peti peti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't duplicate code that we already have in haskellPackages. We have a very sophisticated infrastructure for packaging Haskell tools and it's a little frustrating that Hazura re-invents the wheel here instead of using the existing solutions.

@offlinehacker
Copy link
Contributor

@peti can you then suggest what would be the way to package hasura (it's not on hackage and uses very specific version constraints for some packages)?

@ingenieroariel
Copy link
Member Author

ingenieroariel commented Aug 14, 2020

I am happy to put the work to package it differently, what I did at first was just understand what @offlinehacker had done and update it to latest GHC and Hasura. The dance to get the packages just right is very finicky so there would have to be some exceptions where we pull different versions from Hackage to not need to alter Hasura itself.

Here are a few specific examples:

  1. optics-core, the version we get by default is not useful for Hasura, but currently nixpkgs also has optics-core_0_3_0_1 and we were able to do something like:

optics-core = super.optics-core_0_3_0_1;

  1. cron, the version we get by default is not useful for Hasura, if we update it nixpkgs wide then other things could break, so we created a local file cron.nix with
mkDerivation {
  pname = "cron";
  version = "0.7.0";
  sha256 = "sha256-pc1Ixv0RlDCqj70FllSAbw85FkLfA1FtmzAh369YEjk=";
  ...
}

and then used it like:
cron = self.callPackage ./cron.nix { };

  1. The package was specifying bounds but it really wasn't necessary since it compiled with the newer ones:
    superbuffer = dontCheck (doJailbreak (unmarkBroken super.superbuffer));

Notice that this uses the advanced machinery already in Nixpkgs for Haskell (all credit goes to the original author) so it is not necessarily reinventing the wheel, but I do agree that it would be better to do away with the cron.nix pattern if we can find a better way to override just version numbers, I just need some direction.

@ingenieroariel
Copy link
Member Author

@peti Here is some example code from GHC 8.10 currently on master that seems to use two of the patterns used in hasura.nix:

  # Deviate from Stackage LTS-15.x to fix the build.
  haddock-library = self.haddock-library_1_9_0;

  # Jailbreak to fix the build.
  base-noprelude = doJailbreak super.base-noprelude;
  pandoc = doJailbreak super.pandoc;
  system-fileio = doJailbreak super.system-fileio;
  unliftio-core = doJailbreak super.unliftio-core;

  # Use the latest version to fix the build.
  dhall = self.dhall_1_34_0;
  lens = self.lens_4_19_2;
  optics-core = self.optics-core_0_3_0_1;
  repline = self.repline_0_4_0_0;
  singletons = self.singletons_2_7;
  th-desugar = self.th-desugar_1_11;

# Deviate from Stackage LTS-15.x to fix the build.

@ingenieroariel
Copy link
Member Author

Looked at other packages here and found this, instead of making the versions Reflex wanted available in Nixpkgs, this pulled patches made to the upstream to support the versions in Nixpkgs. This may require help from someone with better Haskell chops than mine, but it is not out of the question:

  reflex = appendPatches super.reflex [
    # Upstream PR: https://github.com/reflex-frp/reflex/pull/434
    # Bump version bounds
    (pkgs.fetchpatch {
      url = "https://github.com/reflex-frp/reflex/commit/e6104bdfd7f664f524b6765275490722e376df4d.patch";
      sha256 ="1awp5p4640cnhfd50dplsvp0kzy6h8r0hpbw1s40blni74r3dhzr";
    })
    # Upstream PR: https://github.com/reflex-frp/reflex/pull/436
    # Fix build with newest dependent-map version
    (pkgs.fetchpatch {
      url = "https://github.com/reflex-frp/reflex/commit/dc3bf44d822d70594e3c474fe3869261776c3554.patch";
      sha256 ="0rbjfj9b8p6zkvd5j4pak5kpgard6cyfvzk750s4xwpc1v84iiqd";
    })
    # Upstream PR: https://github.com/reflex-frp/reflex/pull/437
    # Fix tests with newer dep versions
    (pkgs.fetchpatch {
      url = "https://github.com/reflex-frp/reflex/commit/87c74a1b9d9098eae8a56148c59ed4963a5232c2.patch";
      sha256 ="0qhjjgd6n4fms1hpbblny78c95bfh74izhx9dvrdlnhz6q7xlm9q";
    })
  ];

https://github.com/NixOS/nixpkgs/blame/99ec1661420a1c2cba1e012380dd6344bb640799/pkgs/development/haskell-modules/configuration-common.nix#L1252

@ingenieroariel
Copy link
Member Author

I refactored the code to have graphql-engine and other haskell packages correctly as libraries and made the code that compiles the application consist of only jailbreaks and small mods, it is in essence the same code that was running before but makes the packages available for others.

@peti Does this satisfy your concerns?

@offlinehacker
Copy link
Contributor

@ingenieroariel how did you generate hackage-packages.nix, since I dont see any changes to configuration-hackage2nix.yaml ?

@ingenieroariel
Copy link
Member Author

Ha, manually of course, and after reading your question realizing that was the wrong way ...

... back to reading mode :)

@offlinehacker
Copy link
Contributor

The issue I see is that hackage2nix can only generate packages from hackage as far as I am aware.

@peti is there any way to use git repo for a package not published on hackage when using hackage2nix?

@ingenieroariel
Copy link
Member Author

Ok, so now the process seems to be:

#1. Create a new PR with the following change
#2 Wait a day for it to be applied
#3 Update this PR and take the hasura packages out again and back to the folder where they were?:
Is there a "manual haskell packages" file where I could put them instead?

diff --git a/pkgs/development/haskell-modules/configuration-hackage2nix.yaml b/pkgs/development/haskell-modules/configuration-hackage2nix.yaml
index 586415a1db8..715f14c5b95 100644
--- a/pkgs/development/haskell-modules/configuration-hackage2nix.yaml
+++ b/pkgs/development/haskell-modules/configuration-hackage2nix.yaml
@@ -2611,6 +2611,9 @@ extra-packages:
   - yesod-persistent < 1.5              # pre-lts-11.x versions neeed by git-annex 6.20180227
   - yesod-static ^>= 1.5                # pre-lts-11.x versions neeed by git-annex 6.20180227
   - yesod-test ^>= 1.5                  # pre-lts-11.x versions neeed by git-annex 6.20180227
+  - immortal == 0.2.2.1                 # required by Hasura
+  - dependent-map == 0.2.4.0            # required by Hasura
+  - dependent-sum == 0.4.0              # required by Hasura

 package-maintainers:
   peti:

@offlinehacker
Copy link
Contributor

In non-hackage-packages.nix move to separate files, also rebase. But thanks, this is getting better 🙂

@ingenieroariel
Copy link
Member Author

I can move it to separate files, but in which folder? The same one?

@offlinehacker
Copy link
Contributor

I would just put them in hasura folder for now.

@ingenieroariel ingenieroariel force-pushed the hasura-enable-update branch 2 times, most recently from ad33e69 to e10a1f5 Compare August 14, 2020 20:18
@ingenieroariel
Copy link
Member Author

@GrahamcOfBorg build haskell.packages.ghc8101.graphql-engine
@GrahamcOfBorg build haskell.packages.ghc8101.insert-ordered-containers
@GrahamcOfBorg build haskell.packages.ghc8101.HTF

@ingenieroariel
Copy link
Member Author

Ok, this is really interesting, it seems my work only applies to the executable, but the libraries I touched do not build on their own neither in ghc88 or ghc810 (for different reasons):

nix-build -A haskell.packages.ghc8101graphql-engine
gives

[169 of 187] Compiling Hasura.GraphQL.Execute.LiveQuery.Poll ( src-lib/Hasura/GraphQL/Execute/LiveQuery/Poll.hs, dist/build/Hasura/GraphQL/Execute/LiveQuery/Poll.p_o )

src-lib/Hasura/GraphQL/Execute/LiveQuery/Poll.hs:57:1: error:
    Could not find module ‘GHC.AssertNF’
    Perhaps you haven't installed the profiling libraries for package ‘ghc-heap-view-0.6.2’?
    Use -v (or `:set -v` in ghci) to see a list of the files searched for.
   |
57 | import           GHC.AssertNF
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

but
nix-build -A hasura-graphql-engine
gives

/nix/store/wrc35jvllm3d4lpc6djzrvwif0q4lvy0-graphql-engine-1.3.1-beta1

@ingenieroariel
Copy link
Member Author

The executable package was successful:

https://github.com/NixOS/nixpkgs/pull/95369/checks?check_run_id=1057653764

But the libraries were not successful on the default compiler (ghc88x) and the target one (ghc810x).

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

causes regressions in cachix and hercules-ci-agent

https://github.com/NixOS/nixpkgs/pull/95369
2 packages failed to build:
cachix hercules-ci-agent

3 packages built:
hasura-cli hasura-graphql-engine neuron-notes

cc @domenkozar

failure:

builder for '/nix/store/x74iz00q1rzqbnf3if0wjvs8in9rf1w8-optics-extra-0.3.drv' failed with exit code 1; last 10 log lines:
    die', called at libraries/Cabal/Cabal/Distribution/Simple/Configure.hs:1022:20 in Cabal-3.0.1.0:Distribution.Simple.Configure
    configureFinalizedPackage, called at libraries/Cabal/Cabal/Distribution/Simple/Configure.hs:475:12 in Cabal-3.0.1.0:Distribution.Simple.Configure
    configure, called at libraries/Cabal/Cabal/Distribution/Simple.hs:625:20 in Cabal-3.0.1.0:Distribution.Simple
    confHook, called at libraries/Cabal/Cabal/Distribution/Simple/UserHooks.hs:65:5 in Cabal-3.0.1.0:Distribution.Simple.UserHooks
    configureAction, called at libraries/Cabal/Cabal/Distribution/Simple.hs:180:19 in Cabal-3.0.1.0:Distribution.Simple
    defaultMainHelper, called at libraries/Cabal/Cabal/Distribution/Simple.hs:116:27 in Cabal-3.0.1.0:Distribution.Simple
    defaultMain, called at Setup.hs:4:8 in main:Main
  Setup: Encountered missing or private dependencies:
  optics-core >=0.3 && <0.3.1
cannot build derivation '/nix/store/hxh2fn8vb3i2lr9rskv7b0pw8w279ys0-insert-ordered-containers-0.2.3.1.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/1hvhik5ng6cxpiimhycw4fl3cx26vryk-swagger2-2.5.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/a7c6v7fls9cb0yq1sa2p7xl6arqxcv57-servant-swagger-1.1.7.1.drv': 2 dependencies couldn't be built
cannot build derivation '/nix/store/1aij6rjwx1m1kmqkmjl8d1gpxp6k7678-servant-swagger-ui-core-0.3.3.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/f23x9461awib975ji6fbya01g40fjbk5-servant-auth-swagger-0.2.10.0.drv': 2 dependencies couldn't be built
cannot build derivation '/nix/store/h19a18vgqj00jyz7pdwc098prw73sllz-cachix-api-0.4.0.drv': 4 dependencies couldn't be built
cannot build derivation '/nix/store/ixbv571inhlc9bba8sxqcbw2agbxrg14-hercules-ci-api-core-0.1.1.0.drv': 4 dependencies couldn't be built
cannot build derivation '/nix/store/8adg3a8injii6rc5mhx962zs64gc8sxk-cachix-0.3.8.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/qd7d4vrglx0v0hmg1s5kxl2msxlmwjqj-cachix-0.3.8.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/i96w3d8nids2lhz0dzyiig477xlr2lgj-hercules-ci-api-agent-0.2.2.0.drv': 5 dependencies couldn't be built
cannot build derivation '/nix/store/ricjji66azz0881q3rj9rglxmk209afa-hercules-ci-agent-0.7.4.drv': 4 dependencies couldn't be built

@maralorn
Copy link
Member

maralorn commented Sep 2, 2020

Okay, this sucks, but all overrides that break packages which normally build on 8.8 need to be moved to the 8.10 file. This concerns at least insert-ordered-containers, which as a dep of cachix should really not get broken.

@@ -297,7 +297,6 @@ self: super: {
hsbencher = dontCheck super.hsbencher;
hsexif = dontCheck super.hsexif;
hspec-server = dontCheck super.hspec-server;
HTF = dontCheck super.HTF;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, why do you remove this? This introduces a build error for this package. Just not changing this line could fix the HTF package. Or is there some reason I don‘t get? Maybe then you can even remove some of the dontChecks below for which broken HTF is actually the reason? Or is HTF broken either way already before this PR? Anyway just let me now, what your rationale here was.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was a mistake, I see no rationale for removing it. Sending in a new version of this PR.

@ingenieroariel
Copy link
Member Author

Things I tested locally:

nix-build -A haskell.packages.ghc884.graphql-engine:
Fails with -> Module ‘System.IO.Error’ does not export ‘isResourceVanishedError’.

I tried making base = self.base-compat; but it did not work: https://hackage.haskell.org/package/base-compat-0.11.1/docs/System-IO-Error-Compat.html
nix-build -A haskellPackages.cachix
-> /nix/store/a1mjx07gni83n011b2max5hfq3qbaxff-cachix-0.3.8
nix-build -A haskellPackages.hercules-ci-agent
/nix/store/haqfir8haqwl6kx6423ib2grjf8cmjhz-hercules-ci-agent-0.7.4
nix-build -A haskell.packages.ghc8101.graphql-engine
/nix/store/jdvdaayc73fimvqy8d0545z2kmxirc9s-graphql-engine-1.3.1-beta1
nix-build -A haskellPackages.HFT
gives -> <no location info>: error:
    `cc' failed in phase `C Compiler'. (Exit code: 1)

@ingenieroariel ingenieroariel force-pushed the hasura-enable-update branch 3 times, most recently from aba9f07 to 097b305 Compare September 3, 2020 17:28
@ingenieroariel
Copy link
Member Author

@GrahamcOfBorg build haskellPackages.HTF
@GrahamcOfBorg build haskellPackages.monoidal-containers
@GrahamcOfBorg build haskellPackages.insert-ordered-containers
@GrahamcOfBorg build haskellPackages.list-t
@GrahamcOfBorg build haskellPackages.superbuffer
@GrahamcOfBorg build haskellPackages.stm-containers
@GrahamcOfBorg build haskellPackages.Spock-core
@GrahamcOfBorg build haskellPackages.graphql-engine
@GrahamcOfBorg build haskellPackages.graphql-parser
@GrahamcOfBorg build haskellPackages.ci-info
@GrahamcOfBorg build haskellPackages.pg-client
@GrahamcOfBorg build hasura-graphql-engine
@GrahamcOfBorg build hasura-cli

@GrahamcOfBorg build cachix
@GrahamcOfBorg build hercules-ci-agent

@GrahamcOfBorg build haskell.packages.ghc8101.graphql-engine
@GrahamcOfBorg build haskell.packages.ghc884.graphql-engine

@maralorn
Copy link
Member

maralorn commented Sep 3, 2020

Okay, monoidal-containers is as of last night broken on haskell-updates, I fixed that.

@GrahamcOfBorg build haskellPackages.monoidal-containers
@GrahamcOfBorg build hasura-graphql-engine
@GrahamcOfBorg build haskellPackages.graphql-parser

@ingenieroariel
Copy link
Member Author

@GrahamcOfBorg build haskellPackages.monoidal-containers
@GrahamcOfBorg build hasura-graphql-engine
@GrahamcOfBorg build haskellPackages.graphql-parser

@maralorn
Copy link
Member

maralorn commented Sep 3, 2020

Let's just check everything once again. Just to be sure. I guess most of it will be cached anyways.

@GrahamcOfBorg build haskellPackages.monoidal-containers
@GrahamcOfBorg build haskellPackages.insert-ordered-containers
@GrahamcOfBorg build haskellPackages.list-t
@GrahamcOfBorg build haskellPackages.superbuffer
@GrahamcOfBorg build haskellPackages.stm-containers
@GrahamcOfBorg build haskellPackages.Spock-core
@GrahamcOfBorg build haskellPackages.graphql-engine
@GrahamcOfBorg build haskellPackages.graphql-parser
@GrahamcOfBorg build haskellPackages.ci-info
@GrahamcOfBorg build haskellPackages.pg-client
@GrahamcOfBorg build hasura-graphql-engine
@GrahamcOfBorg build hasura-cli

@GrahamcOfBorg build cachix
@GrahamcOfBorg build hercules-ci-agent

@GrahamcOfBorg build haskell.packages.ghc8101.graphql-engine
@GrahamcOfBorg build haskell.packages.ghc884.graphql-engine

@maralorn
Copy link
Member

maralorn commented Sep 3, 2020

Result of nixpkgs-review pr 95369 1

2 packages built:
- hasura-cli
- hasura-graphql-engine

@ingenieroariel
Copy link
Member Author

@GrahamcOfBorg build haskellPackages.monoidal-containers
@GrahamcOfBorg build haskellPackages.insert-ordered-containers
@GrahamcOfBorg build haskellPackages.list-t
@GrahamcOfBorg build haskellPackages.superbuffer
@GrahamcOfBorg build haskellPackages.stm-containers
@GrahamcOfBorg build haskellPackages.Spock-core
@GrahamcOfBorg build haskellPackages.graphql-engine
@GrahamcOfBorg build haskellPackages.graphql-parser
@GrahamcOfBorg build haskellPackages.ci-info
@GrahamcOfBorg build haskellPackages.pg-client
@GrahamcOfBorg build hasura-graphql-engine
@GrahamcOfBorg build hasura-cli

@GrahamcOfBorg build cachix
@GrahamcOfBorg build hercules-ci-agent

@GrahamcOfBorg build haskell.packages.ghc8101.graphql-engine
@GrahamcOfBorg build haskell.packages.ghc884.graphql-engine

@maralorn maralorn dismissed peti’s stale review September 3, 2020 20:27

Requirements fulfilled

@ingenieroariel ingenieroariel removed the request for review from cdepillabout September 3, 2020 20:29
@maralorn maralorn merged commit c024a1b into NixOS:haskell-updates Sep 3, 2020
@maralorn
Copy link
Member

maralorn commented Sep 3, 2020

Thanks, for sticking with it 'til the end! Great work!

@datakurre
Copy link
Contributor

datakurre commented Sep 25, 2020

@ingenieroariel @maralorn Something happened to the closure size of hasura-graphql-engine in this merge. A xz packaged closure at piens/nixpkgs:2b0b078a64162ab193f2aeb1c88bd2e92f42e90c was about 20MB. Now, at nixos/nixpkgs-channels:nixos-20.09 (d447429) it is above 400MB.

@maralorn
Copy link
Member

Oh, well. That is a finicky thing. I guess somewhere something introduced a runtime dependency on the Haskell ecosystem.

I honestly don‘t know what the best practices are to prevent such a thing.

If we know what the issue is we can probably fix something with references-to or simpler, but somebody will have to compare those two closures and it's dependency trees to find out where the issue lies.

@maralorn
Copy link
Member

On second though:

This PR dropped the justStaticExecutables for hasura-graphql-engine. Probably it's just that? I didn‘t think about closure sizes at that time. Lesson learned.

If some could try that out, that would be great. Otherwise open an issue so we don‘t forget about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants