cabal icon indicating copy to clipboard operation
cabal copied to clipboard

new-build --enable-benchmarks does not cause benchmarks to be built

Open mrkkrp opened this issue 6 years ago • 18 comments

Describe the bug

Passing --enable-benchmarks to new-build does not cause benchmarks to be built.

To Reproduce

Here is Travis CI log for megaparsec which has broken benchmarks but still it's all green:

https://travis-ci.org/mrkkrp/megaparsec/jobs/585309471#L296

The exact command in this case is:

cabal new-build megaparsec megaparsec-tests --enable-tests --enable-benchmarks --flags=dev

yet even simpler command also doesn't build benchmarks:

cabal new-build --enable-benchmarks

It returns with exit code 0 even though the benchmarks are broken and do not compile.

Please use version-prefixed commands (e.g. v2-build or v1-build) to avoid ambiguity.

I got at first a different result with v2-build: it failed as it should. Then after that new-build also started to fail. Then later it started to report that the build is "up to date" returning 0 (the code is still broken).

Expected behavior

When --enable-benchmarks is passed, all benchmarks should be built and if they do not compile we should reliably get non-zero exit status no matter what.

System informataion

  • Operating system: NixOS
  • cabal version 2.4.1.0
  • ghc version 8.6.5

mrkkrp avatar Sep 25 '19 15:09 mrkkrp

Here we can see clearly that despite --enable-benchmarks no benchmarks are considered for building even though the megaparsec package has two benchmarks: bench-speed and bench-memory.

mrkkrp avatar Sep 25 '19 15:09 mrkkrp

nevermind; disregard the comment below -- I misunderstood your report at first


@mrkkrp Btw, cabal new-build w/o any target is context-specific (i.e. CWD specific;similar to how make works)

iow, does the all target (i.e. "all enabled components") build them

cabal v2-build --enable-benchmark all

or specifically only the benchmarks via the special target

cabal v2-build --enable-benchmark all:benches

which denotes the enabled benchmarks of all components do what you expect?

The target syntax is documented also at https://cabal.readthedocs.io/en/latest/nix-local-build.html#cabal-v2-build

hvr avatar Sep 25 '19 16:09 hvr

@mrkkrp ok, here's what's going on:

the target megaparsec refers to the library component of megaparsec, not to the package megaparsec

in other words, the statement

cabal new-build megaparsec megaparsec-tests --enable-tests --enable-benchmarks --flags=dev

gets resolved into

cabal new-build lib:megaparsec lib:megaparsec-tests --enable-tests --enable-benchmarks --flags=dev

due to the target-resolution rules in the light of ambiguities; whereas you intended it to denote

cabal new-build :pkg:megaparsec :pkg:megaparsec-tests --enable-tests --enable-benchmarks --flags=dev

does this make any sense?

Fwiw, is there any reason you didn't simply use

cabal new-build all --enable-tests --enable-benchmarks --flags=dev

which doesn't require you to enumerate the local packages explicitly?

hvr avatar Sep 25 '19 16:09 hvr

I don't think I knew about the all magic.

Why do test suites get built though? They are other components, different from libraries, just like benchmarks. What cofuses me now is that test suites do get built, but not benchmarks.

mrkkrp avatar Sep 25 '19 17:09 mrkkrp

@mrkkrp you're right; something's inconsistent here; the target doesn't seem to be resolved the way I described it... needs more investigation

hvr avatar Sep 25 '19 20:09 hvr

Just hit this, i think for the third time And nowadays things are worse cause you can skip the all target, cabal build --enable-benchmarks does not build local packages benchmarks but cabal build all --enable-benchmarks continue doing it as described above

jneira avatar Jan 10 '22 22:01 jneira

I just double checked in the hls repo:

PS D:\hls> cabal build  --enable-benchmarks
Warning: Requested index-state 2021-12-29T12:30:08Z is newer than
'hackage.haskell.org'! Falling back to older state (2021-12-29T11:45:44Z).
Resolving dependencies...
Build profile: -w ghc-8.10.7 -O1
In order, the following will be built (use -v for more details):
 - ghcide-1.5.0.1 (exe:ghcide-test-preprocessor) (first run)
 - hie-compat-0.2.1.0 (lib) (first run)
 - hls-graph-1.5.1.1 (lib) (first run)
 - hiedb-0.4.1.0 (lib) (requires build)
 - hls-plugin-api-1.2.0.2 (lib) (first run)
 - ghcide-1.5.0.1 (lib) (first run)
.... hls plugins
 - haskell-language-server-1.5.1.0 (lib) (first run)
 - hls-refine-imports-plugin-1.0.0.2 (lib) (first run)
 - haskell-language-server-1.5.1.0 (exe:haskell-language-server-wrapper) (first run)
 - haskell-language-server-1.5.1.0 (exe:haskell-language-server) (first run)
 - haskell-language-server-1.5.1.0 (test:wrapper-test) (first run)
 - haskell-language-server-1.5.1.0 (test:func-test) (first run)
......

PS D:\hls> cabal build  all --enable-benchmarks
Build profile: -w ghc-8.10.7 -O1
In order, the following will be built (use -v for more details):
 - ghcide-1.5.0.1 (exe:ghcide-test-preprocessor) (first run)
 - hie-compat-0.2.1.0 (lib) (first run)
 - hls-graph-1.5.1.1 (lib) (first run)
 - hiedb-0.4.1.0 (lib) (requires build)
 - hls-plugin-api-1.2.0.2 (lib) (first run)
 - ghcide-1.5.0.1 (lib) (first run)
 - hls-test-utils-1.1.0.2 (lib) (first run)
... hls plugins
 - haskell-language-server-1.5.1.0 (lib) (first run)
 - ghcide-1.5.0.1 (exe:ghcide) (first run)
 - cubicbezier-0.6.0.6 (lib) (requires build)
 - diagrams-lib-1.4.5.1 (lib) (requires build)
 ... hls plugin tests
 - haskell-language-server-1.5.1.0 (exe:haskell-language-server-wrapper) (first run)
 - ghcide-1.5.0.1 (test:ghcide-tests) (first run)
 - ghcide-1.5.0.1 (exe:ghcide-bench) (first run)
 - hls-refine-imports-plugin-1.0.0.2 (test:tests) (first run)
 - haskell-language-server-1.5.1.0 (exe:haskell-language-server) (first run)
 - haskell-language-server-1.5.1.0 (test:wrapper-test) (first run)
 - haskell-language-server-1.5.1.0 (test:func-test) (first run)
 - shake-bench-0.1.0.2 (lib) (first run)
 - ghcide-1.5.0.1 (bench:benchHist) (first run)
.....

You can see in the second build and not in the first one shake-bench a local package used by ghcide-1.5.0.1 (bench:benchHist)

jneira avatar Jan 10 '22 22:01 jneira

In continuation of https://github.com/haskell/cabal/issues/6259#issuecomment-1009390936.

In HLS we observed:

cabal build all --project-file=cabal-ghc901.project

Fails (at the state of https://github.com/haskell/haskell-language-server/commit/4ffdf45a390e93d02f5fb05ac8abd604becb809e).

The .project uses -stylishhaskell cabal flag:

flag stylishHaskell
  description: Enable stylishHaskell plugin
  default:     True
  manual:      True

to disable the hls-stylish-haskell-plugin, because stylish-haskell 0.13 (while has base <5), in reality does not support 9.0.1 & 9.2.1.

In CI dev workflows we running cabal build --project-file=cabal-ghc901.project as a base, so the builds & tests & benchmarks for all enabled targets to succeed. While the cabal all overrules all flag & .project configuration & starts to target & build the hls-stylish-haskell-plugin & so starts to build stylish-haskell 0.13 on unsupported GHC versions, which of course results in the error return code for the build. This overruling of .project settings was weird to observe. It is probably just a note may belong into a separate report or may be "works as intended".

The raw log of discussion in HLS: https://matrix.to/#/!oOjZFsoNYPAbTEgSOA:libera.chat/$0oqtvFoHlPe1At_LwJSQKrfboCKDcMvimyZUt8DS_jE?via=libera.chat&via=matrix.org&via=monoid.al

Anton-Latukha avatar Jan 13 '22 17:01 Anton-Latukha

Correction about https://github.com/haskell/cabal/issues/6259#issuecomment-1009390936

  • cabal build is not equivalent to cabal build all (yet). I think we talked about make them equivalent at some point but it is not implemented
  • It seems in hls we have a top level package (hls itself) and cabal build is equivalent to cabal build pkg:haskell-language-server
    • it turns out that hat top level package has the rest of local packages as dependencies so build hls make build all subpackages libs
    • the layout is somewhat similar to megaparsec, which has a top level package megaparsec and a subpackage megaparsec-tests (but our top level package has no benchmarks)

So sorry for the confusion, as penitence will try to reproduce the original issue in megaparsec with last cabal

jneira avatar Jan 14 '22 08:01 jneira

I can reproduce the first case with a recent cabal built from master:

D:\ws\haskell\issues\megaparsec>cabal v2-build megaparsec megaparsec-tests --enable-tests --enable-benchmarks --flags=dev -w ghc-8.10.7 --dry-run
....
 - megaparsec-tests-9.2.0 (lib) (first run)
 - megaparsec-tests-9.2.0 (test:tests) (first run)
  • also reproduced for cabal v2-build megaparsec(without megaparsec-tests)
D:\ws\haskell\issues\megaparsec>cabal v2-build megaparsec --enable-tests --enable-benchmarks --flags=dev -w ghc-8.10.7 --dry-run
....
 - megaparsec-tests-9.2.0 (lib) (first run)
  • but no for cabal build --enable-benchmarks
D:\ws\haskell\issues\megaparsec>cabal v2-build --enable-benchmarks -w ghc-8.10.7
 --dry-run
Build profile: -w ghc-8.10.7 -O1
In order, the following would be built (use -v for more details):
 - megaparsec-9.2.0 (lib) (first run)
 - megaparsec-9.2.0 (bench:bench-memory) (first run)
 - megaparsec-9.2.0 (bench:bench-speed) (first run)

So the bug is still present and it seems nowadays cabal build without targets is not exactly equivalent to use explicit targets

jneira avatar Jan 14 '22 08:01 jneira

Hi, I just spent a while trying to debug a CI script of mine. Turns out cabal build <my-package> --enable-benchmarks doesn't build benchmarks 😓.

I see this issue is labeled blocked: info-needed. Can I help collect info?

mitchellwrosen avatar Dec 30 '23 16:12 mitchellwrosen

I don't think its actually info-needed at this point -- we do need to figure out the underlying cause and design.

But the specific issue seems to be that cabal new-build is not equivalent to cabal new-build all and not equivalent to cabal new-build pkg-name but does something slightly different.

So someone digging in and figuring out what exactly cabal new-build does without an explicit target would seem to be the most important thing, and then based on that we can reach a consensus on what it should do.

gbaz avatar Dec 30 '23 22:12 gbaz

I'm going to look into this.

alt-romes avatar Jan 31 '24 16:01 alt-romes

Here's a little investigation, on a clean tmp project with a default testsuite and basic benchmark component:

(1) Without listing a package, everything works as it probably should:

  • cabal build: builds the library only
  • cabal build --enable-tests: builds the library and the testsuite
  • cabal build --enable-benchmarks: builds the library and the benchmark
  • cabal build --enable-tests --enable-benchmarks: builds the library, testsuite, and benchmark

(2) However, in passing a pkg as an argument

  • cabal build <my-pkg>: builds the library only
  • cabal build <my-pkg> --enable-tests: builds the library only
  • cabal build <my-pkg> --enable-benchmarks: builds the library only
  • cabal build <my-pkg> --enable-tests --enable-benchmarks: also builds the library only The conclusion is that specifying <my-pkg> makes cabal-install ignore enable benchmarks or tests flags, likely because it considers to be the library component, and not the package.

Perhaps the name resolution logic here could account for --enable-{bench,test} flags and consider the argument a package if yes.

Defaulting the argument to a package likely entails often building more things than necessary, and will change existing setups and workflows assuming this behaviour, but trying to be a bit smarter when in presence of such flags seems like a good compromise.

(PS: about the megaparsec and megaparsec-tests package, the reason it builds tests but not benchmarks is likely because the benchmark is a component within one of those two packages whose name doesn't match the package name, thus doesn't get built because we fall into the second case)

alt-romes avatar Jan 31 '24 17:01 alt-romes

Perhaps a good first step in this issue is to give a warning to the user if they only specify selectors which resolve to non-benchmark components? Perhaps there should be a new selector for "all components from a package", perhaps that already exists and the error can suggest it.

I think the selector syntax/logic should be kept as simple as possible and probably consistent with what already happens .

mpickering avatar Feb 01 '24 10:02 mpickering

There is a selector for all benchmarks:

https://github.com/haskell/cabal/blob/17217c3a0459235502bb92a4b17c8fe82672eab6/cabal-install/tests/IntegrationTests2.hs#L198

philderbeast avatar Feb 01 '24 11:02 philderbeast

I encountered a similar situation around installation:

.. warning::

  If not every package has an executable to install, use ``all:exes`` rather
  than ``all`` as the target.

https://github.com/haskell/cabal/commit/fbdcad02e22db97372919b02d6a1670a81be1dc2#diff-7366bf7292133a6cd2005aed5db013b2774af3c8d49c8b7e20ec5e8e03e1ff2eR296-R300

philderbeast avatar Feb 01 '24 11:02 philderbeast

I cannot reproduce the behavior you described @alt-romes. Specifically, you say that cabal build <my-pkg> --enable-benchmarks will build only the library, however that is not true in this example:

❯ rm -rf dist-newstyle

ouroboros-consensus on main via λ 9.6.4
➜ cabal build ouroboros-consensus --enable-benchmarks --dry-run
Resolving dependencies...
Build profile: -w ghc-9.6.4 -O1
In order, the following would be built (use -v for more details):
 - strict-sop-core-0.1.0.0 (lib) (first run)
 - sop-extras-0.1.0.0 (lib) (first run)
 - ouroboros-consensus-0.15.0.0 (lib) (first run)
 - ouroboros-consensus-0.15.0.0 (lib:unstable-tutorials) (first run)
 - ouroboros-consensus-0.15.0.0 (lib:unstable-mempool-test-utils) (first run)
 - ouroboros-consensus-0.15.0.0 (lib:unstable-consensus-testlib) (first run)
 - ouroboros-consensus-0.15.0.0 (lib:unstable-mock-block) (first run)
 - ouroboros-consensus-0.15.0.0 (bench:mempool-bench) (first run)
 - ouroboros-consensus-0.15.0.0 (bench:ChainSync-client-bench) (first run)

This is on main, with this patch:

diff --git a/cabal.project b/cabal.project
index c4f84c2dd..e3eb698c1 100644
--- a/cabal.project
+++ b/cabal.project
@@ -27,7 +27,7 @@ packages:
   strict-sop-core

 -- We want to always build the test-suites and benchmarks
-tests: true
-benchmarks: true
+-- tests: true
+-- benchmarks: true

 import: ./asserts.cabal

Which makes it as a "clean project". Just used that project because I had it at hand.

Also checked that is disabled in global config:

ouroboros-consensus on  main [$+?] via λ 9.6.4
➜ cat $(cygpath -m "C:\Users\Javier\AppData\Roaming\cabal\config") | grep tests
-- tests: False
-- run-tests:
  -- tests: False
  -- tests:

ouroboros-consensus on  main [$+?] via λ 9.6.4
➜ cat $(cygpath -m "C:\Users\Javier\AppData\Roaming\cabal\config") | grep benchmarks
-- benchmarks: False
  -- benchmarks: False

jasagredo avatar Feb 01 '24 19:02 jasagredo