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

haskellPackages.shellFor: improve documentation #102598

Conversation

cdepillabout
Copy link
Member

Motivation for this change

I was trying to figure out a problem with benchmarks not being included in shellFor (per #102323 from @harendra-kumar).

It took me a while to just figure out how shellFor works. I did a small refactoring, and I added some documentation to make it easier to understand. I think this would be an improvement on the current state of the code.

None of the functionality should be changed.

cc @maralorn and @peti

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 cdepillabout force-pushed the documentation-for-shellFor-rebased branch from 6181b0d to 6b8d245 Compare November 3, 2020 07:55
@cdepillabout
Copy link
Member Author

cdepillabout commented Nov 3, 2020

I also have a local commit that adds support for benchmark stuff (which will fix #102323). I'll send this as a follow-up PR after this current PR gets merged in:

diff --git a/pkgs/development/haskell-modules/make-package-set.nix b/pkgs/development/haskell-modules/make-package-set.nix
index c5604aa4196..8201f0d3aff 100644
--- a/pkgs/development/haskell-modules/make-package-set.nix
+++ b/pkgs/development/haskell-modules/make-package-set.nix
@@ -293,6 +293,10 @@ in package-set { inherit pkgs stdenv callPackage; } self // {
       , # Whether or not to generated a Hoogle database for all the
         # dependencies.
         withHoogle ? false
+      , # Whether or not to include benchmark dependencies of your local
+        # packages.  You should set this to true if you have benchmarks defined
+        # in your local packages that you want to be able to run with cabal benchmark
+        doBenchmark ? false
       , ...
       } @ args:
       let
@@ -398,7 +402,11 @@ in package-set { inherit pkgs stdenv callPackage; } self // {
           version = "0";
           license = null;
         }
-        // packageInputs;
+        // packageInputs
+        // pkgs.lib.optionalAttrs doBenchmark {
+          # `doBenchmark` needs to explicitly be set here because haskellPackages.mkDerivation defaults it to `false`.  If the user wants benchmark dependencies included in their development shell, it has to be explicitly enabled here.
+          doBenchmark = true;
+        };
 
         # This is a pseudo Haskell package derivation that contains all the
         # dependencies for the packages in `selected`.
@@ -418,9 +426,11 @@ in package-set { inherit pkgs stdenv callPackage; } self // {
         # `haskellPackages.mkDerivation`).
         #
         # pkgWithCombinedDepsDevDrv :: Derivation
         pkgWithCombinedDepsDevDrv = pkgWithCombinedDeps.envFunc { inherit withHoogle; };
 
-        mkDerivationArgs = builtins.removeAttrs args [ "packages" "withHoogle" ];
+        mkDerivationArgs = builtins.removeAttrs args [ "packages" "withHoogle" "doBenchmark" ];
 
       in pkgWithCombinedDepsDevDrv.overrideAttrs (old: mkDerivationArgs // {
         nativeBuildInputs = old.nativeBuildInputs ++ mkDerivationArgs.nativeBuildInputs or [];

@cdepillabout
Copy link
Member Author

@GrahamcOfBorg build tests.haskell-shellFor

@cdepillabout
Copy link
Member Author

I get the same hash when building tests.haskell-shellFor on both this branch and haskell-updates, so that is a strong indicator that I didn't actually change any of the functionality in shellFor in this PR.

@harendra-kumar
Copy link

@cdepillabout my first change was exactly the same change as the one you wrote in #102598 (comment) . But then I later changed it to what posted in #102323 . I thought that made it simpler. Any reasons for preferring one over the other?

Copy link
Member

@maralorn maralorn left a comment

Choose a reason for hiding this comment

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

I am too tired right now, too fully grasp every of your comments. But it seems nice.

pkgs/development/haskell-modules/make-package-set.nix Outdated Show resolved Hide resolved
pkgs/development/haskell-modules/make-package-set.nix Outdated Show resolved Hide resolved
pkgs/development/haskell-modules/make-package-set.nix Outdated Show resolved Hide resolved
pkgs/development/haskell-modules/make-package-set.nix Outdated Show resolved Hide resolved
Co-authored-by: maralorn <malte.brandy@maralorn.de>
@cdepillabout cdepillabout merged commit ff7f4a8 into NixOS:haskell-updates Nov 4, 2020
@cdepillabout cdepillabout deleted the documentation-for-shellFor-rebased branch November 4, 2020 00:48
@cdepillabout
Copy link
Member Author

cdepillabout commented Nov 4, 2020

Thanks for the review!


@harendra-kumar

Any reasons for preferring one over the other?

The only reason is that I wanted to explicitly document it.

Also, those args are passed to envFrom, which is just a stdenv.mkDerivation, not haskellPackages.mkDerivation. stdenv.mkDerivation doesn't know anything about doBenchmark (that's a haskellPackages.mkDerivation thing).

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

3 participants