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.

Sign up for GitHub

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

Jump to bottom

Fix haskellPackages.dhall_1_28_0 and spago #75931

Closed
wants to merge 2 commits into from
Closed

Fix haskellPackages.dhall_1_28_0 and spago #75931

wants to merge 2 commits into from

Conversation

ijaketak
Copy link
Contributor

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 @

cdepillabout
cdepillabout reviewed Dec 19, 2019
View reviewed changes
pkgs/development/haskell-modules/configuration-nix.nix Outdated
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

cdepillabout
cdepillabout reviewed Dec 19, 2019
View reviewed changes
pkgs/development/tools/purescript/spago/default.nix Outdated
@@ -18,6 +18,7 @@ mkDerivation {
rev = "a4679880402ead320f8be2f091b25d30e27b62df";
fetchSubmodules = true;
};
patches = [ ./spago-0.12.1-dhall-1.28.0.patch ];
Copy link
Member

Choose a reason for hiding this comment

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

This file is generated with cabal2nix.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

cdepillabout
cdepillabout reviewed Dec 19, 2019
View reviewed changes
pkgs/development/tools/purescript/spago/spago-0.12.1-dhall-1.28.0.patch Outdated
@@ -0,0 +1,122 @@
--- a/src/Spago/Config.hs
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@ijaketak Please comment that on top of the patch.

cdepillabout
cdepillabout requested changes Dec 19, 2019
View reviewed changes
Copy link
Member

@cdepillabout cdepillabout left a comment

Choose a reason for hiding this comment

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

@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

@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

@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
Copy link
Contributor Author

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

nh2
nh2 reviewed Dec 19, 2019
View reviewed changes
Copy link
Contributor

@nh2 nh2 left a comment

Choose a reason for hiding this comment

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

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.

pkgs/development/haskell-modules/configuration-nix.nix Outdated
# 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; };
Copy link
Contributor

Choose a reason for hiding this comment

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

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; };

pkgs/development/haskell-modules/configuration-nix.nix Outdated
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@cdepillabout
Copy link
Member

cdepillabout commented Dec 20, 2019 •
edited

@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
Copy link
Contributor Author

@cdepillabout No, please take over.

@ijaketak
haskellPackages.dhall_1_28_0: depends on prettyprinter-1.5.1
669e9b5 
dhall-lang/dhall-haskell@dedd5e0
This raises the lower bound on prettyprinter to 1.5.1 since
`removeTrailingWhitespace` is buggy in earlier versions.
@ijaketak
spago: port to dhall-1.28.0
24670ee
@cdepillabout cdepillabout mentioned this pull request Dec 20, 2019
Merged
10 tasks
@cdepillabout
Copy link
Member

cdepillabout commented Dec 20, 2019 •
edited

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
@cdepillabout @ijaketak
haskellPackages.dhall_1_28_0: fix overrides so it builds
077103a 
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 pushed a commit that referenced this pull request Dec 26, 2019
@cdepillabout @ijaketak @peti
haskellPackages.dhall_1_28_0: fix overrides so it builds
4d2624b 
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 pushed a commit that referenced this pull request Dec 27, 2019
@cdepillabout @ijaketak @peti
haskellPackages.dhall_1_28_0: fix overrides so it builds
db1adfe 
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
Reviewers

@nh2 nh2 nh2 left review comments

@cdepillabout cdepillabout cdepillabout requested changes

@basvandijk basvandijk Awaiting requested review from basvandijk

Assignees
No one assigned
Labels
6.topic: haskell 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

haskellPackages.dhall_1_28_0: fails to build
3 participants
@ijaketak @cdepillabout @nh2