haskell lib: add --enable-benchmarks in doBenchmark #46512

michaelpj · 2018-09-11T09:13:10Z

Motivation for this change

doBenchmark adds the dependencies for benchmarks to be built, but does not actually build them. This is surprising - I would think that anyone setting this flag wants the build to check that the benchmarks build. They still aren't executed, but that's expensive and should probably indeed be done separately.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option 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/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

basvandijk · 2018-09-11T09:55:23Z

pkgs/development/haskell-modules/lib.nix

@@ -106,11 +106,11 @@ rec {
  /* doBenchmark enables dependency checking, compilation and execution
     for benchmarks listed in the package description file.
   */
-  doBenchmark = drv: overrideCabal drv (drv: { doBenchmark = true; });
+  doBenchmark = drv: appendConfigureFlag (overrideCabal drv (drv: { doBenchmark = true; })) "--enable-benchmarks";


I think we should move this logic to the generic-builder.nix so that users who override doBenchmark manually (i.e. without using doBenchmark) also get the correct behavior.

The reason I thought we maybe shouldn't do that is that it's not clear that dontBenchmark will work properly in that case if the package specifies --enable-benchmarks, i.e. we'd also need to remove it from configureFlags.

What do you mean with "if the package specifies --enable-benchmarks"? AFAIK cabal packages can only define benchmarks, not instruct the build system to build them. I think we should treat doBenchmark the same as doCheck.

Perhaps I'm confused - but the generic builder also takes configureFlags, so if those contain --enable-benchmarks then it would fail weirdly, right?

Yes you're right. Do note that doCheck has the same kind of issue:

$ nix-build -E 'let pkgs = import <nixpkgs> {}; in with pkgs.haskell.lib; dontCheck (appendConfigureFlag pkgs.haskellPackages.scientific "--enable-tests")' these derivations will be built: /nix/store/cq07i85kxsxkgayrzg2rcipnd4qr4qm6-scientific-0.3.5.2.drv building '/nix/store/cq07i85kxsxkgayrzg2rcipnd4qr4qm6-scientific-0.3.5.2.drv'... setupCompilerEnvironmentPhase Build with /nix/store/3v8p0pvcplr3bfvrnh4fyqrqk2ddyl6f-ghc-8.2.2. ignoring (possibly broken) abi-depends field for packages unpacking sources unpacking source archive /nix/store/cwcqnj4h1fcjnl01igxsxf7sr0dq4c2m-scientific-0.3.5.2.tar.gz source root is scientific-0.3.5.2 setting SOURCE_DATE_EPOCH to timestamp 1504095418 of file scientific-0.3.5.2/test/test.hs patching sources Replace Cabal file with edited version from http://hackage.haskell.org/package/scientific-0.3.5.2/revision/5.cabal. compileBuildDriverPhase setupCompileFlags: -package-db=/tmp/nix-build-scientific-0.3.5.2.drv-0/package.conf.d -j8 -threaded [1 of 1] Compiling Main ( Setup.hs, /tmp/nix-build-scientific-0.3.5.2.drv-0/Main.o ) Linking Setup ... configuring configureFlags: --verbose --prefix=/nix/store/9yyq4xqh379bc6i9rr3jw8kp7ll1mwz8-scientific-0.3.5.2 --libdir=$prefix/lib/$compiler --libsubdir=$pkgid --docdir=/nix/store/fj6ijj02hflkbjgh7s2vw23wpx7vvvpl-scientific-0.3.5.2-doc/share/doc --with-gcc=gcc --package-db=/tmp/nix-build-scientific-0.3.5.2.drv-0/package.conf.d --ghc-option=-optl=-Wl,-rpath=/nix/store/9yyq4xqh379bc6i9rr3jw8kp7ll1mwz8-scientific-0.3.5.2/lib/ghc-8.2.2/scientific-0.3.5.2 --ghc-option=-j8 --disable-split-objs --disable-library-profiling --disable-profiling --enable-shared --disable-coverage --enable-library-vanilla --enable-executable-dynamic --disable-tests --ghc-option=-split-sections --enable-tests --extra-lib-dirs=/nix/store/069g827lh3hrhp4vkcq3rsh5jh65pm3l-ncurses-6.0-20171125/lib --extra-lib-dirs=/nix/store/8qfd8gx0j3yzamkrbrfz5kc00h4cqd1q-gmp-6.1.2/lib --extra-lib-dirs=/nix/store/069g827lh3hrhp4vkcq3rsh5jh65pm3l-ncurses-6.0-20171125/lib Configuring scientific-0.3.5.2... CallStack (from HasCallStack): die', called at libraries/Cabal/Cabal/Distribution/Simple/Configure.hs:948:20 in Cabal-2.0.1.0:Distribution.Simple.Configure configureFinalizedPackage, called at libraries/Cabal/Cabal/Distribution/Simple/Configure.hs:470:12 in Cabal-2.0.1.0:Distribution.Simple.Configure configure, called at libraries/Cabal/Cabal/Distribution/Simple.hs:570:20 in Cabal-2.0.1.0:Distribution.Simple confHook, called at libraries/Cabal/Cabal/Distribution/Simple/UserHooks.hs:67:5 in Cabal-2.0.1.0:Distribution.Simple.UserHooks configureAction, called at libraries/Cabal/Cabal/Distribution/Simple.hs:174:19 in Cabal-2.0.1.0:Distribution.Simple defaultMainHelper, called at libraries/Cabal/Cabal/Distribution/Simple.hs:119:27 in Cabal-2.0.1.0:Distribution.Simple defaultMain, called at Setup.hs:2:8 in main:Main Setup: Encountered missing dependencies: QuickCheck >=2.5 && <2.12, smallcheck >=1.0 && <1.2, tasty >=0.5 && <1.1, tasty-ant-xml >=1.0 && <1.2, tasty-hunit >=0.8 && <0.11, tasty-quickcheck >=0.8 && <0.10, tasty-smallcheck >=0.2 && <0.9 builder for '/nix/store/cq07i85kxsxkgayrzg2rcipnd4qr4qm6-scientific-0.3.5.2.drv' failed with exit code 1 error: build of '/nix/store/cq07i85kxsxkgayrzg2rcipnd4qr4qm6-scientific-0.3.5.2.drv' failed

Do you think this is an issue in practice?

I do think it's important that we treat doCheck and doBenchmarks in the same way.

Good point! If they're broken in the same way then I'm okay with this. I'll change the PR to do it the way you suggest.

basvandijk · 2018-09-11T15:15:20Z

pkgs/development/haskell-modules/lib.nix

@@ -106,11 +106,11 @@ rec {
  /* doBenchmark enables dependency checking, compilation and execution
     for benchmarks listed in the package description file.
   */
-  doBenchmark = drv: overrideCabal drv (drv: { doBenchmark = true; });
+  doBenchmark = drv: appendConfigureFlag (overrideCabal drv (drv: { doBenchmark = true; })) "--enable-benchmarks";


Yes you're right. Do note that doCheck has the same kind of issue:

$ nix-build -E 'let pkgs = import <nixpkgs> {}; in with pkgs.haskell.lib; dontCheck (appendConfigureFlag pkgs.haskellPackages.scientific "--enable-tests")' these derivations will be built: /nix/store/cq07i85kxsxkgayrzg2rcipnd4qr4qm6-scientific-0.3.5.2.drv building '/nix/store/cq07i85kxsxkgayrzg2rcipnd4qr4qm6-scientific-0.3.5.2.drv'... setupCompilerEnvironmentPhase Build with /nix/store/3v8p0pvcplr3bfvrnh4fyqrqk2ddyl6f-ghc-8.2.2. ignoring (possibly broken) abi-depends field for packages unpacking sources unpacking source archive /nix/store/cwcqnj4h1fcjnl01igxsxf7sr0dq4c2m-scientific-0.3.5.2.tar.gz source root is scientific-0.3.5.2 setting SOURCE_DATE_EPOCH to timestamp 1504095418 of file scientific-0.3.5.2/test/test.hs patching sources Replace Cabal file with edited version from http://hackage.haskell.org/package/scientific-0.3.5.2/revision/5.cabal. compileBuildDriverPhase setupCompileFlags: -package-db=/tmp/nix-build-scientific-0.3.5.2.drv-0/package.conf.d -j8 -threaded [1 of 1] Compiling Main ( Setup.hs, /tmp/nix-build-scientific-0.3.5.2.drv-0/Main.o ) Linking Setup ... configuring configureFlags: --verbose --prefix=/nix/store/9yyq4xqh379bc6i9rr3jw8kp7ll1mwz8-scientific-0.3.5.2 --libdir=$prefix/lib/$compiler --libsubdir=$pkgid --docdir=/nix/store/fj6ijj02hflkbjgh7s2vw23wpx7vvvpl-scientific-0.3.5.2-doc/share/doc --with-gcc=gcc --package-db=/tmp/nix-build-scientific-0.3.5.2.drv-0/package.conf.d --ghc-option=-optl=-Wl,-rpath=/nix/store/9yyq4xqh379bc6i9rr3jw8kp7ll1mwz8-scientific-0.3.5.2/lib/ghc-8.2.2/scientific-0.3.5.2 --ghc-option=-j8 --disable-split-objs --disable-library-profiling --disable-profiling --enable-shared --disable-coverage --enable-library-vanilla --enable-executable-dynamic --disable-tests --ghc-option=-split-sections --enable-tests --extra-lib-dirs=/nix/store/069g827lh3hrhp4vkcq3rsh5jh65pm3l-ncurses-6.0-20171125/lib --extra-lib-dirs=/nix/store/8qfd8gx0j3yzamkrbrfz5kc00h4cqd1q-gmp-6.1.2/lib --extra-lib-dirs=/nix/store/069g827lh3hrhp4vkcq3rsh5jh65pm3l-ncurses-6.0-20171125/lib Configuring scientific-0.3.5.2... CallStack (from HasCallStack): die', called at libraries/Cabal/Cabal/Distribution/Simple/Configure.hs:948:20 in Cabal-2.0.1.0:Distribution.Simple.Configure configureFinalizedPackage, called at libraries/Cabal/Cabal/Distribution/Simple/Configure.hs:470:12 in Cabal-2.0.1.0:Distribution.Simple.Configure configure, called at libraries/Cabal/Cabal/Distribution/Simple.hs:570:20 in Cabal-2.0.1.0:Distribution.Simple confHook, called at libraries/Cabal/Cabal/Distribution/Simple/UserHooks.hs:67:5 in Cabal-2.0.1.0:Distribution.Simple.UserHooks configureAction, called at libraries/Cabal/Cabal/Distribution/Simple.hs:174:19 in Cabal-2.0.1.0:Distribution.Simple defaultMainHelper, called at libraries/Cabal/Cabal/Distribution/Simple.hs:119:27 in Cabal-2.0.1.0:Distribution.Simple defaultMain, called at Setup.hs:2:8 in main:Main Setup: Encountered missing dependencies: QuickCheck >=2.5 && <2.12, smallcheck >=1.0 && <1.2, tasty >=0.5 && <1.1, tasty-ant-xml >=1.0 && <1.2, tasty-hunit >=0.8 && <0.11, tasty-quickcheck >=0.8 && <0.10, tasty-smallcheck >=0.2 && <0.9 builder for '/nix/store/cq07i85kxsxkgayrzg2rcipnd4qr4qm6-scientific-0.3.5.2.drv' failed with exit code 1 error: build of '/nix/store/cq07i85kxsxkgayrzg2rcipnd4qr4qm6-scientific-0.3.5.2.drv' failed

Do you think this is an issue in practice?

I do think it's important that we treat doCheck and doBenchmarks in the same way.

michaelpj · 2018-09-11T15:25:21Z

Updated - in fact this is clearly the right place to do it.

michaelpj · 2018-09-11T15:25:31Z

(given all the other things doing similar stuff here)

michaelpj · 2018-09-18T10:30:27Z

ping?

basvandijk · 2018-09-18T13:04:28Z

Thanks!

michaelpj requested review from basvandijk, peti and ryantm as code owners September 11, 2018 09:13

GrahamcOfBorg added 6.topic: haskell 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Sep 11, 2018

basvandijk suggested changes Sep 11, 2018

View reviewed changes

haskell generic builder: enable benchmarks if doBenchmark is true

175c4f0

michaelpj force-pushed the imp/haskell-benchmarks branch from ed29fe3 to 175c4f0 Compare September 11, 2018 15:24

GrahamcOfBorg added 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ and removed 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Sep 11, 2018

basvandijk approved these changes Sep 18, 2018

View reviewed changes

basvandijk merged commit 0a30853 into NixOS:master Sep 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

haskell lib: add --enable-benchmarks in doBenchmark #46512

haskell lib: add --enable-benchmarks in doBenchmark #46512

michaelpj commented Sep 11, 2018

basvandijk Sep 11, 2018

michaelpj Sep 11, 2018

basvandijk Sep 11, 2018

michaelpj Sep 11, 2018

basvandijk Sep 11, 2018

michaelpj Sep 11, 2018

basvandijk Sep 11, 2018

michaelpj commented Sep 11, 2018

michaelpj commented Sep 11, 2018

michaelpj commented Sep 18, 2018

basvandijk commented Sep 18, 2018

haskell lib: add --enable-benchmarks in doBenchmark #46512

haskell lib: add --enable-benchmarks in doBenchmark #46512

Conversation

michaelpj commented Sep 11, 2018

Motivation for this change

Things done

basvandijk Sep 11, 2018

Choose a reason for hiding this comment

michaelpj Sep 11, 2018

Choose a reason for hiding this comment

basvandijk Sep 11, 2018

Choose a reason for hiding this comment

michaelpj Sep 11, 2018

Choose a reason for hiding this comment

basvandijk Sep 11, 2018

Choose a reason for hiding this comment

michaelpj Sep 11, 2018

Choose a reason for hiding this comment

basvandijk Sep 11, 2018

Choose a reason for hiding this comment

michaelpj commented Sep 11, 2018

michaelpj commented Sep 11, 2018

michaelpj commented Sep 18, 2018

basvandijk commented Sep 18, 2018