Squelched warning output when building multiple packages #2545

Closed
sw17ch opened this Issue Aug 30, 2016 · 10 comments

Projects

None yet

6 participants

@sw17ch
sw17ch commented Aug 30, 2016 edited

Hi All,

Warning output is squelched when referring to external locations. Warning output is produced on the console when -Werror is present. The following pair of projects demonstrates the problem on the latest version of stack (1.1.2).

STACK_TEST.zip

This pair of projects was produced and tested with the following sequence of actions.

Check your stack version.

$ stack --version
Version 1.1.2 x86_64 hpack-0.14.0

Create two new projects.

stack new stacktest
stack new stackdep

Edit the stack.yaml file in stacktest to match the following.

resolver: lts-6.14

packages:
- '.'
- location: ../stackdep

extra-deps: []

flags: {}

extra-package-dbs: []

Edit src/Lib.hs in stacktest to match the following.

module Lib
    ( someFunc
    ) where

thisShouldGenerateAWarning = undefined

someFunc :: IO ()
someFunc = putStrLn "someFunc"

Edit stacktest.cabal to resemble the following. Of note is the ghc-options line.

$ cat stacktest/stacktest.cabal
name:                stacktest
version:             0.1.0.0
synopsis:            Initial project template from stack
description:         Please see README.md
homepage:            https://github.com/sw17ch/stacktest#readme
license:             BSD3
license-file:        LICENSE
author:              John Van Enk
maintainer:          sw17ch@gmail.com
copyright:           2016 John Van Enk
category:            Web
build-type:          Simple
-- extra-source-files:
cabal-version:       >=1.10

library
  hs-source-dirs:      src
  exposed-modules:     Lib
  build-depends:       base >= 4.7 && < 5
  default-language:    Haskell2010
  ghc-options:         -Wall

EXPECTED: two warnings in the build output of src/Lib.hs when building stacktest.

    /Users/johnvanenk/STACK_TEST/stacktest/src/Lib.hs:5:1: Warning:
        Defined but not used: ‘thisShouldGenerateAWarning’

    /Users/johnvanenk/STACK_TEST/stacktest/src/Lib.hs:5:1: Warning:
        Top-level binding with no type signature:
          thisShouldGenerateAWarning :: forall t. t

ACTUAL: no warning output is emitted.

$ stack build
stackdep-0.1.0.0: unregistering (local file changes: src/StackDep.hs stackdep.cabal)
stacktest-0.1.0.0: configure
stacktest-0.1.0.0: build
stackdep-0.1.0.0: configure
stacktest-0.1.0.0: copy/register
stackdep-0.1.0.0: build
stackdep-0.1.0.0: copy/register
Completed 2 action(s).

Further notes.

The warnings are still present in the log files, but no indications that warnings exist in the build are present on the console.

$ grep -Irn Warning .
./.stack-work/logs/stacktest-0.1.0.0.log:5:src/Lib.hs:5:1: Warning:
./.stack-work/logs/stacktest-0.1.0.0.log:6:    Defined but not used: ‘thisShouldGenerateAWarning’
./.stack-work/logs/stacktest-0.1.0.0.log:8:src/Lib.hs:5:1: Warning:
./.stack-work/logs/stacktest-0.1.0.0.log:10:      thisShouldGenerateAWarning :: forall t. t

Furthermore, turning on -Werror produces the warning output as expected, but is sometimes awkward when doing large refactors.

This behavior does not appear to exist when using extra-deps without a package in some other location as specified by a location directive.

@Blaisorblade
Collaborator
Blaisorblade commented Aug 30, 2016 edited

Thanks for the report. In that scenario stack should silence warnings for the external dependencies, but this seems* to overreach.
But the internal logic seems another one: stack prints warnings to the console if there's a single package being built—or do I misunderstand? What happens if you first build dependencies and then your package?

Technically, my first guess is: is this code the culprit?

    console = wanted
           && all (\(ActionId ident _) -> ident == taskProvides) (Set.toList acRemaining)
           && eeTotalWanted == 1
@dysinger dysinger assigned dysinger and unassigned dysinger Aug 30, 2016
@borsboom
Contributor

This may be a duplicate of #426 and #298.

@dysinger
Contributor

I was able to reproduce this also - even without the "location" qualifier on one of the projects.

test:

% cd /tmp
% mkdir testymctestcase
% cd testymctestcase
% stack new thinga
% stack new thingb
% cat >stack.yaml <<\EOF
resolver: lts-6.14
packages:
  - "thinga"
  - "thingb"
ghc-options:
  '*' : '-Wall'
EOF
% echo 'main = undefined' | tee thinga/app/Main.hs
% stack build
... no warnings ...
% 
@borsboom borsboom changed the title from Squelched Warning Output When Depending on External Locations to Squelched warning output when building multiple packages in parallel Aug 30, 2016
@borsboom borsboom changed the title from Squelched warning output when building multiple packages in parallel to Squelched warning output when building multiple packages Aug 30, 2016
@borsboom
Contributor

The #426 (--ghc-output) solution is not all that discoverable; a user would need to know to use it in order to see warnings. Potentially Stack could detect warnings in GHC's output when it's squelched due to parallel builds (there's already some logic for detecting warnings in order to munge filenames into absolute paths) and if it sees any dump that build's output to the terminal. We'd have to make sure this doesn't end up consuming a lot of memory or otherwise hurt performance though.

@borsboom borsboom added this to the P1: Must milestone Sep 14, 2016
@borsboom borsboom added the type: bug label Sep 14, 2016
@ppelleti

#426 is a bit different, in that it wants a realtime indication of progress. It seems to be stuck in concerns about interleaved output.

The problem with warnings (this bug) could be solved by waiting for the build to finish, and then dumping the log files to stderr for all local packages that were built.

@snoyberg
Contributor

I opened up #2594 with a dump-logs config option/flag. I'd consider it a fix for #426, and presumably this issue too.

@snoyberg snoyberg added a commit that referenced this issue Sep 15, 2016
@snoyberg snoyberg Do not run tests and benchmarks for local extra-dep pkgs
This logic works, but arguably is not being handled very elegantly. I
see two alternatives:

* Do a more serious refactoring to unify Hackage extra-deps and local
  extra-dep values
* In the `case mtarget`, add `_ | lpvExtraDep lpv -> mempty`. I tried
  this at first, but I believe the behavior was slightly confusing,
  since running `stack build :test-suite-name` for a local extra-dep
  didn't run the test. However, that may in fact be the desired
  behavior.

This and the previous commit appear to fix #2545
e3a0502
@snoyberg
Contributor

Also of note: I've opened up #2596, which ensures that local extra-dep packages are in fact treated as dependencies for both purposes of stack test and for deciding whether or not to send build output to the console.

@borsboom
Contributor

@snoyberg: perhaps what you did in #2596 could be adapted to fully solve this issue by having default behaviour that detects any warnings in the logfile and dump it if any are found. We already have some code to detect warnings in GHC output in order to make displayed file paths absolute.

At least, I think that would be desirable default behaviour. Any other opinions here?

@snoyberg
Contributor

I think #2596 should be treated as an orthogonal bug fix which restores correct behavior for dependency vs non-dependency packages. I'd actually consider this closer to the behavior change being done in #2594, and it sounds like we could have three settings of dumping logs:

  • Dump all logs for targets, all the time
  • Never dump logs for targets
  • Dump logs for targets when the logs have warnings in them

It would certainly be easy to extend the dump-logs code I already wrote to account for this.

@snoyberg snoyberg added a commit that closed this issue Sep 19, 2016
@snoyberg snoyberg Do not run tests and benchmarks for local extra-dep pkgs
This logic works, but arguably is not being handled very elegantly. I
see two alternatives:

* Do a more serious refactoring to unify Hackage extra-deps and local
  extra-dep values
* In the `case mtarget`, add `_ | lpvExtraDep lpv -> mempty`. I tried
  this at first, but I believe the behavior was slightly confusing,
  since running `stack build :test-suite-name` for a local extra-dep
  didn't run the test. However, that may in fact be the desired
  behavior.

This and the previous commit appear to fix #2545
e3a0502
@snoyberg snoyberg closed this in e3a0502 Sep 19, 2016
@borsboom borsboom reopened this Sep 19, 2016
@borsboom
Contributor

[reopened since warning output is still "squelched" by default when building multiple packages]

@borsboom borsboom added a commit that closed this issue Sep 30, 2016
@borsboom borsboom Dump build logs if they contain warnings (fixes #2545)
Also munge the build output when dumping logs e.g. so that file paths
are made absolute and TH loading messages are squelched.
1892042
@borsboom borsboom closed this in 1892042 Sep 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment