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

Fix haskellPackages.dhall_1_28_0 and spago #75931

Closed
wants to merge 2 commits into from

Conversation

@ijaketak
Copy link
Contributor

@ijaketak ijaketak commented Dec 19, 2019

Motivation for this change

Fix #75930. See also purescript/spago#529

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 @

@ofborg ofborg bot added the 6.topic: haskell label Dec 19, 2019
@@ -637,6 +637,18 @@ self: super: builtins.intersectAttrs super {
cairo = addBuildTool super.cairo self.buildHaskellPackages.gtk2hs-buildtools;
pango = disableHardening (addBuildTool super.pango self.buildHaskellPackages.gtk2hs-buildtools) ["fortify"];

# https://github.com/dhall-lang/dhall-haskell/pull/1422

This comment has been minimized.

@cdepillabout

cdepillabout Dec 19, 2019
Member

Could you expand this comment so that the nixpkgs maintainers will have a better idea when these overrides can be removed (without having to follow the link)?

This comment has been minimized.

@ijaketak

ijaketak Dec 19, 2019
Author Contributor

Done. Link was wrong.
dhall-lang/dhall-haskell@dedd5e0

@@ -18,6 +18,7 @@ mkDerivation {
rev = "a4679880402ead320f8be2f091b25d30e27b62df";
fetchSubmodules = true;
};
patches = [ ./spago-0.12.1-dhall-1.28.0.patch ];

This comment has been minimized.

@cdepillabout

cdepillabout Dec 19, 2019
Member

This file is generated with cabal2nix.

Could you instead add these patches to pkgs/developement/haskell/configuration-common.nix?

This comment has been minimized.

@ijaketak

ijaketak Dec 19, 2019
Author Contributor

Done.

@@ -0,0 +1,122 @@
--- a/src/Spago/Config.hs

This comment has been minimized.

@cdepillabout

cdepillabout Dec 19, 2019
Member

If possible, we generally don't like to carry around patches in the nixpkgs repo, but instead fetch them from upstream.

Could you pull this patch directly from upstream and apply it using fetchPatch. If you grep through pkgs/development/haskell-modules/configuration-common.nix you should be able to find examples of people pulling in patches from GitHub PRs.

This comment has been minimized.

@ijaketak

ijaketak Dec 19, 2019
Author Contributor

purescript/spago#529 cannot be applied for spago-0.12.1. different patch required.

This comment has been minimized.

@nh2

nh2 Dec 19, 2019
Contributor

@ijaketak Please comment that on top of the patch.

Copy link
Member

@cdepillabout cdepillabout left a comment

@ijaketak Thanks a lot for trying to fix up spago here.

If you take care of the above problems, we can merge this in.

Alternatively, it may just be a lot easier to create a dhall_1_27_0 derivation and setup spago so it uses it. See #75449 (comment) where I describe how to do this.

@cdepillabout
Copy link
Member

@cdepillabout cdepillabout commented Dec 19, 2019

@ijaketak Could you change this line to be dhall = dhall_1_27_0; (or dhall_1_28_0 depending on what you choose to do)?

https://github.com/NixOS/nixpkgs/pull/75931/files#diff-607a8b2e94748382228f44039b753cf0R660

I messed this up when I originally submitted the PR adding spago.

@cdepillabout
Copy link
Member

@cdepillabout cdepillabout commented Dec 19, 2019

@ijaketak I got spago building statically. I did it by just using dhall_1_27_0:

purescript/spago#528 (comment)

This was pretty easy. I recommend you do the same for this PR.

@ijaketak ijaketak force-pushed the ijaketak:spago-0.12.1-dhall-1.28.0 branch from 9044bab to d008f9e Dec 19, 2019
@ijaketak
Copy link
Contributor Author

@ijaketak ijaketak commented Dec 19, 2019

Let me conclude that this PR is completed as dhall_1_28_0 is fixed and spago is compatible with this version.

Copy link
Contributor

@nh2 nh2 left a comment

I found two places that should be self instead of super (so that they can be further overridden).

Beyond that and adding a comment for the reason of the patch, this change looks good to me.

# https://github.com/dhall-lang/dhall-haskell/commit/dedd5e0ea6fd12f87d887af3d2220eebc61ee8af
dhall_1_28_0 =
let
prettyprinter-ansi-terminal = super.prettyprinter-ansi-terminal.override { prettyprinter = super.prettyprinter_1_5_1; };

This comment has been minimized.

@nh2

nh2 Dec 19, 2019
Contributor

Suggested change
prettyprinter-ansi-terminal = super.prettyprinter-ansi-terminal.override { prettyprinter = super.prettyprinter_1_5_1; };
prettyprinter-ansi-terminal = super.prettyprinter-ansi-terminal.override { prettyprinter = self.prettyprinter_1_5_1; };
prettyprinter-ansi-terminal = super.prettyprinter-ansi-terminal.override { prettyprinter = super.prettyprinter_1_5_1; };
in
super.dhall_1_28_0.override {
prettyprinter = super.prettyprinter_1_5_1;

This comment has been minimized.

@nh2

nh2 Dec 19, 2019
Contributor

Suggested change
prettyprinter = super.prettyprinter_1_5_1;
prettyprinter = self.prettyprinter_1_5_1;

This comment has been minimized.

@ijaketak

ijaketak Dec 20, 2019
Author Contributor

Done.

@ijaketak ijaketak force-pushed the ijaketak:spago-0.12.1-dhall-1.28.0 branch from e29b8f2 to 24f2b17 Dec 20, 2019
@cdepillabout
Copy link
Member

@cdepillabout cdepillabout commented Dec 20, 2019

@ijaketak I'd like to bump spago to 0.13.0, since a new version was just released. Do you mind if I take over this PR for now?

I'll get dhall-1.28.0 building (basically using what you currently have here), as well as fix up spago to 0.13.0.

@ijaketak ijaketak force-pushed the ijaketak:spago-0.12.1-dhall-1.28.0 branch from 24f2b17 to eaf6782 Dec 20, 2019
@ijaketak
Copy link
Contributor Author

@ijaketak ijaketak commented Dec 20, 2019

@cdepillabout No, please take over.

ijaketak added 2 commits Dec 19, 2019
dhall-lang/dhall-haskell@dedd5e0
This raises the lower bound on prettyprinter to 1.5.1 since
`removeTrailingWhitespace` is buggy in earlier versions.
@ijaketak ijaketak force-pushed the ijaketak:spago-0.12.1-dhall-1.28.0 branch from eaf6782 to 24670ee Dec 20, 2019
@cdepillabout
Copy link
Member

@cdepillabout cdepillabout commented Dec 20, 2019

I sent #76079 using the stuff from this PR to get dhall_1_28_0 building.

I also sent #76084 updating to spago-0.13.0.

cdepillabout added a commit that referenced this pull request Dec 20, 2019
This PR fixes dhall_1_28_0, dhall-bash_1_0_25, and dhall-json_1_6_0 so
they build.

They all require a newer version of prettyprinter than we get from the
LTS package set.

This is from #75931 by @ijaketak.

Co-authored-by: Keito Kajitani <ijaketak@gmail.com>
@ijaketak ijaketak closed this Dec 20, 2019
peti added a commit that referenced this pull request Dec 26, 2019
This PR fixes dhall_1_28_0, dhall-bash_1_0_25, and dhall-json_1_6_0 so
they build.

They all require a newer version of prettyprinter than we get from the
LTS package set.

This is from #75931 by @ijaketak.

Co-authored-by: Keito Kajitani <ijaketak@gmail.com>
peti added a commit that referenced this pull request Dec 26, 2019
This PR fixes dhall_1_28_0, dhall-bash_1_0_25, and dhall-json_1_6_0 so
they build.

They all require a newer version of prettyprinter than we get from the
LTS package set.

This is from #75931 by @ijaketak.

Co-authored-by: Keito Kajitani <ijaketak@gmail.com>
peti added a commit that referenced this pull request Dec 27, 2019
This PR fixes dhall_1_28_0, dhall-bash_1_0_25, and dhall-json_1_6_0 so
they build.

They all require a newer version of prettyprinter than we get from the
LTS package set.

This is from #75931 by @ijaketak.

Co-authored-by: Keito Kajitani <ijaketak@gmail.com>
peti added a commit that referenced this pull request Dec 27, 2019
This PR fixes dhall_1_28_0, dhall-bash_1_0_25, and dhall-json_1_6_0 so
they build.

They all require a newer version of prettyprinter than we get from the
LTS package set.

This is from #75931 by @ijaketak.

Co-authored-by: Keito Kajitani <ijaketak@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.