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

haskell-lib: Create flipped version of haskell-lib #100732

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@expipiplus1
Copy link
Contributor

@expipiplus1 expipiplus1 commented Oct 16, 2020

More or less closes #37177

Created with this hacky script: https://github.com/expipiplus1/nix-flipper

WIP, could use a bit of neatening, and probably some checking!

  • At the moment the output from nix-flipper is tweaked a little bit,
    would it be worth making it 100% automated to ease keeping this up to
    date?
  • Worth implementing lib in terms of lib-flipped?
  • Is haskell.lib.flipped a better place an haskell.lib-flipped
  • Was tempting to call this bil.nix
Motivation for this change
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 nixpkgs-review --run "nixpkgs-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.
@cdepillabout
Copy link
Member

@cdepillabout cdepillabout commented Oct 17, 2020

I've wanted this as well, but I'm somewhat concerned about maintaining two similar apis.

Let's see what @maralorn and @peti think as well.

@maralorn
Copy link
Member

@maralorn maralorn commented Oct 17, 2020

I don‘t have a strong opinion about this. On the one hand this is neat.

On the other hand I think the haskell.lib is already a very crowded place and this are a lot lines of code. And it never occured to me to be annoyed by the order of arguments. I think if we had the $ or the . operators from Haskell in nix I would think more strongly about this, but since we have to explicitly bracket anyways I don‘t see a huge advantage.

So, yeah, not sure … (But I don‘t intend and don‘t feel like I have the authority to veto this.)

Maybe if someone showed me some real examples of code that would be much prettier with this I would be more enthusiastic.

@expipiplus1
Copy link
Contributor Author

@expipiplus1 expipiplus1 commented Oct 17, 2020

Maybe if someone showed me some real examples of code that would be much prettier with this I would be more enthusiastic.

@maralorn Here's the first example which came to mind: https://github.com/expipiplus1/vulkan/blob/9e3229904c170d1e92c80e574dc610e97e36c1c9/examples/default.nix#L29-L31, but I've bumped into this enough times that it became worth the 30 minutes writing the generation script :)

The irritation here is that one has to read this expression inside out (or outside in) because the addExtraLibrary at the beginning is associated with pkgs.vulkan-headers at the end.

It's not like this is impossible to read, but it is a bit of an irritation.

On the other hand I think the haskell.lib is already a very crowded place and this are a lot lines of code.

somewhat concerned about maintaining two similar apis.

Could certainly be made more compact by having something like unflipped // { appendPatch = x: drv: unflipped.appendPatch drv x; ... } (so only specifying the attributes which need flipping, rather than duplicating everything as in this PR currently). On the other hand, it's not hard to generate totally automatically.

if we had the $ or the . operators from Haskell in nix

Yes please! I suggest to avoid stepping on the existing uses of ..

A more sensible midway point might be to include a function in lib.list, foldEndo :: [a -> a] -> a -> a (named after Haskell's appEndo .: foldMap Endo) (Could be made more loosely typed (obv. with a more appropriate name) given nix's lack of static typing); so the above example could be:

vulkan-utils = foldEndo [
  (addExtraLibrary pkgs.vulkan-headers)
  doHaddock
  disableLibraryProfiling
] (self.callCabal2nix "" ../utils { });

Of course I think this is nicest

vulkan-utils = (addExtraLibrary pkgs.vulkan-headers)  doHaddock
   disableLibraryProfiling $ self.callCabal2nix "" ../utils { };

@maralorn
Copy link
Member

@maralorn maralorn commented Oct 17, 2020

Okay, that example is convincing.

I really like the foldEndo thing. That's the first time, that I like nix list not having parens.

I think in general I am in favor of generating less code. So I have slight preference to the // approach, but I don‘t know if it's worth rewriting it now.

The remaining question is, do we even want functions that can‘t be flipped in the flipped attrSet?

To be honest I wouldn‘t be surprised if @peti had some more concrete opinions about it, so we probably should wait a while for him to answer.

@expipiplus1
Copy link
Contributor Author

@expipiplus1 expipiplus1 commented Oct 17, 2020

I think in general I am in favor of generating less code. So I have slight preference to the // approach, but I don‘t know if it's worth rewriting it now.

If it's best, it shouldn't take more than a few minutes tweaking the script.

The remaining question is, do we even want functions that can‘t be flipped in the flipped attrSet?

I'd like them in there because it means one only has to import haskell.lib.flipped to get everything useful, instead of haskell.lib // haskell.lib.flipped. It does make me think that flipped isn't the most precise name, as it's really "drv-as-last-parameter", this just happens to be a flip in every function so far. I'm not fussed at all about the precise logistics though.

To be honest I wouldn‘t be surprised if @peti had some more concrete opinions about it, so we probably should wait a while for him to answer.

Absolutely, I'm in no hurry at all.

@peti
Copy link
Member

@peti peti commented Oct 19, 2020

I'm not sure whether I see when and where those flipped libraries functions are supposed to be used. I mean, surely it's not a good idea to mix both variants in the same file, is it? So ... since all our files in Nixpkgs are pretty much committed to the original "unflipped" version, where are those flipped variants supposed to be used?

@expipiplus1
Copy link
Contributor Author

@expipiplus1 expipiplus1 commented Oct 19, 2020

since all our files in Nixpkgs are pretty much committed to the original "unflipped" version

Why, I don't think it should be hard to change this? (Not that I'm advocating for any change in nixpkgs).

where are those flipped variants supposed to be used?

In the code of people who might otherwise use haskell.lib (for example the people who posted here #37177).

@maralorn
Copy link
Member

@maralorn maralorn commented Oct 19, 2020

I guess we could start of by importing unflipped qualified. And maybe we could use it in new files or rewrite some of the old. (Although I see the pain point of never knowing which way around the arguments are in a certain file …)

But for me having this for downstream users is already a benefit.

@expipiplus1
Copy link
Contributor Author

@expipiplus1 expipiplus1 commented Oct 19, 2020

I think that switching over everything in nixpkgs at once would be easier than using one-here one-there and having the stress that comes with that. However, I really don't want to distract or gate this PR on its possible application in nixpkgs, it certainly wasn't the intention! :)

@expipiplus1
Copy link
Contributor Author

@expipiplus1 expipiplus1 commented Oct 27, 2020

I've just noticed that checkUnusedPackages already has the drv parameter last! (doesn't work of course, because packunused is broken ...)

Edit: and generateOptparseApplicativeCompletion

@expipiplus1
Copy link
Contributor Author

@expipiplus1 expipiplus1 commented Nov 3, 2020

In the meantime, I've done some work with hnix to make making programs to modify nix code even nicer than the aforementioned hacky script, https://github.com/expipiplus1/update-nix-fetchgit/blob/master/src/Nix/Match/Typed.hs

If a polished script to generate the flipped version is required, it could be done.

@expipiplus1
Copy link
Contributor Author

@expipiplus1 expipiplus1 commented Nov 7, 2020

An example from nixpkgs (haskell-updates branch atm), although it's a bit longer I think it's much easier to see what's going on

{
  update-nix-fetchgit = let deps = [ pkgs.git pkgs.nix pkgs.nix-prefetch-git ];
  in generateOptparseApplicativeCompletion "update-nix-fetchgit" (overrideCabal
    (addTestToolDepends (super.update-nix-fetchgit.overrideScope (self: super: {
      optparse-generic = self.optparse-generic_1_4_4;
      optparse-applicative = self.optparse-applicative_0_16_0_0;
    })) deps) (drv: {
      buildTools = drv.buildTools or [ ] ++ [ pkgs.makeWrapper ];
      postInstall = drv.postInstall or "" + ''
        wrapProgram "$out/bin/update-nix-fetchgit" --prefix 'PATH' ':' "${
          pkgs.lib.makeBinPath deps
        }"
      '';
    }));

  # With flipped functions
  update-nix-fetchgit = let
    deps = [ pkgs.git pkgs.nix pkgs.nix-prefetch-git ];
    compose = foldr (f: g: x: f (g x)) (x: x);
  in compose [
    (generateOptparseApplicativeCompletion "update-nix-fetchgit")
    (addTestToolDepends deps)
    (overrideCabal (drv: {
      buildTools = drv.buildTools or [] ++ [ pkgs.makeWrapper ];
      postInstall = drv.postInstall or "" + ''
        wrapProgram "$out/bin/update-nix-fetchgit" --prefix 'PATH' ':' "${
          pkgs.lib.makeBinPath deps
        }"
      '';
    }))
    (drv:
      drv.overrideScope (self: super: {
        optparse-generic = self.optparse-generic_1_4_4;
        optparse-applicative = self.optparse-applicative_0_16_0_0;
      })
    )
  ] super.update-nix-fetchgit;
}

@expipiplus1
Copy link
Contributor Author

@expipiplus1 expipiplus1 commented Nov 27, 2020

What's the word on this? I'm happy to do whatever cleanup is necessary.

@peti
Copy link
Member

@peti peti commented Nov 27, 2020

I have the following problem. I agree that the flipped style gives nicer code, but IMHO the opportunity to change the functions in Nixpkgs has passed. It's too late. We have tons of code that use the current argument order and we cannot chance our functions without causing a major annoyance to our users. Now, adding a second set of functions that do the same thing but with their arguments reversed feels like a really bad idea. The last thing I want is to have code within Nixpkgs use two different sets of functions that share the same name but have their arguments reversed. That is not going to improve the readability of that code, which kinda defeats the purpose of this whole endeavor.

So, basically, I think that these new functions give nicer code and I am all for people using them, but I am against people using them in Nixpkgs. Which begs the question of whether these functions should be a part of Nixpkgs ... and I tend to think: no, they should not.

Can you see my point?

@expipiplus1
Copy link
Contributor Author

@expipiplus1 expipiplus1 commented Nov 27, 2020

Absolutely, I certainly wouldn't advocate for mixing the two.

I think where we disagree is whether this should be in nixpkgs for all downstream users.

One potential way forward would be to deprecate the use of the current haskell.lib in nixpkgs, and switch over to the flipped set. There are not that many uses over the whole package set.

@peti
Copy link
Member

@peti peti commented Nov 27, 2020

One potential way forward would be to deprecate the use of the current haskell.lib in nixpkgs, and switch over to the flipped set.

Yes, that would be possible. Personally, I don't think the benefits are worth the effort, though. If someone else wants to pick up that torch and run with it, I would not object. I'm just not going to do it myself.

@stale
Copy link

@stale stale bot commented Jun 3, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale label Jun 3, 2021
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.

4 participants