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

haskell modules: Make 17.09 forwards compatible #34557

Merged
merged 3 commits into from Feb 5, 2018
Merged

haskell modules: Make 17.09 forwards compatible #34557

merged 3 commits into from Feb 5, 2018

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Feb 3, 2018

Motivation for this change

Recent commit 8155312 introduced new buildHaskellPackages argument and broke compatibility with 17.09

Now if we call makePackageSet in 17.09 with the new argument it fails with called with unexpected argument ‘buildHaskellPackages’.

This PR adds an ellipsis (...) to prevent called with unexpected argument errors in the future:

error: anonymous function at /nix/store/bfbxsw3jma333xv6x4r2kglwwa3f9zx8-nixpkgs-channels-c1d9aff56e0ae52ee4705440fe09291a51e91977-src/pkgs/development/haskell-modules/make-package-set.nix:4:1 called with unexpected argument ‘buildHaskellPackages’, at /nix/store/bfbxsw3jma333xv6x4r2kglwwa3f9zx8-nixpkgs-channels-c1d9aff56e0ae52ee4705440fe09291a51e91977-src/lib/customisation.nix:74:12
(use ‘--show-trace’ to show detailed location information)

cc @Ericson2314

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@4e6 4e6 requested a review from peti as a code owner February 3, 2018 13:55
@4e6 4e6 mentioned this pull request Feb 3, 2018
Closed
@dhess dhess mentioned this pull request Feb 4, 2018
Closed
@Ericson2314
Copy link
Member

Uh shouldn't this be against 17.09?

@Ericson2314
Copy link
Member

Also please add a comment on each ellipsis saying why. Otherwise looks good, thanks.

@dhess
Copy link
Contributor

dhess commented Feb 5, 2018 •
edited

@Ericson2314 I think the point here is to make sure that you can use makePackageSet (edit without explicitly specifying the new parameter) with either 17.09 or a recent nixpkgs. Right now it's either-or because of the new parameter.

@Ericson2314
Copy link
Member

@dhess yeah but to do that we need to allow passing the extra argument for both, and just have it be unused on, 17.09, so the extra unused parameter or ellipses (I actually prefer extra parameter, come to think of it, too) goes on 17.09.

@dhess
Copy link
Contributor

dhess commented Feb 5, 2018

@Ericson2314 OK, thanks for the clarification. (I am not the author but am eagerly awaiting the fix ;)

@Ericson2314
Copy link
Member

NP. I'd do it myself but I'm on my phone.

peti
peti requested changes Feb 5, 2018
View reviewed changes
pkgs/development/haskell-modules/default.nix Outdated
@@ -6,6 +6,7 @@
, initialPackages ? import ./initial-packages.nix
, configurationCommon ? import ./configuration-common.nix
, configurationNix ? import ./configuration-nix.nix
, ...
Copy link
Member

Choose a reason for hiding this comment

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

Please do not add ellipsis here. I don't want these functions to accept any random argument without an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll use makePackgeSet instead

Copy link
Member

Choose a reason for hiding this comment

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

I am sorry, but that is not what I meant. Please don't accept ... arguments at all. I don't want any of these functions to take random arguments without an error.

@4e6
Copy link
Contributor Author

4e6 commented Feb 5, 2018

I added the comments. and addressed @peti remark

Ellipsis is added to create (some kind of) backward compatibility. With ellipsis, when argument list is extended in master, we can still support both master and release just by passing extra arguments.

@Ericson2314
Copy link
Member

Yeah could you do buildHaskellPackages ? null instead?

@4e6
Copy link
Contributor Author

4e6 commented Feb 5, 2018

Yeah could you do buildHaskellPackages ? null instead?

But it is a required argument, it should never be null. What's wrong with the ellipsis?

@Ericson2314
Copy link
Member

@4e6 you missed me saying this PR should point at release-17.09 instead.

@Ericson2314 Ericson2314 changed the base branch from master to release-17.09 February 5, 2018 15:45
@Ericson2314
Copy link
Member

I went ahead and changed the branch for you.

@Ericson2314 Ericson2314 changed the title Make haskell-modules backward compatible haskell modules: Make 17.09 forwards compatible Feb 5, 2018
peti
peti requested changes Feb 5, 2018
View reviewed changes
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 accept specific arguments rather than using ....

You also need to rebase your PR relative to the current release-17.09 branch.

@4e6 4e6 force-pushed the haskell-modules-compatibility branch from 1363ee5 to df8e9d0 Compare February 5, 2018 18:12
@4e6
Copy link
Contributor Author

4e6 commented Feb 5, 2018

I didn't get that we are talking about merging into release-17.09. Then buildHaskellPackages ? null makes sense. Sorry for the noise.

@Ericson2314
Copy link
Member

Looks great now. Thanks!

@Ericson2314 Ericson2314 merged commit 220b35b into NixOS:release-17.09 Feb 5, 2018
@Ericson2314
Copy link
Member

Oops meant to squash, but it didn't do it. Oh well.

@4e6 4e6 deleted the haskell-modules-compatibility branch February 5, 2018 19:55
@4e6 4e6 mentioned this pull request Feb 13, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peti peti peti requested changes

Assignees
No one assigned
Labels
2.status: work-in-progress 6.topic: haskell 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@4e6 @Ericson2314 @dhess @peti @FRidh @GrahamcOfBorg