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

Duplicate builds with --test #952

Closed
meteficha opened this issue Sep 8, 2015 · 14 comments
Closed

Duplicate builds with --test #952

meteficha opened this issue Sep 8, 2015 · 14 comments
Milestone

Comments

@meteficha
Copy link
Member

Steps to reproduce:

  1. Reproduces both with an existing .stack-work infra and after stack clean && find . -name .stack-work -exec rm -Rf {} \;.
  2. stack build --test --pedantic --no-run-tests

Expected:
One build per package. Every build configured for testing.

Actual:
Packages get built twice: once normally, once for testing.

$ stack --version
Version 0.1.4.1, Git revision e2152eca1bb74c4b8a917c4ad4792b58e6fe8342 X86_64

$ stack clean && find . -name .stack-work -exec rm -Rf {} \;

$ stack build --test --pedantic --no-run-tests 
frontrow-entities-0.0.1.1: configure
frontrow-entities-0.0.1.1: build
frontrow-entities-0.0.1.1: install
fancy-api-0.0.1: configure
fancy-api-0.0.1: build
mixpanel-updater-0.1.0.0: configure
mixpanel-updater-0.1.0.0: build
parent-0.0.0: configure
parent-0.0.0: build
fr-0.1.0.0: configure
fr-0.1.0.0: build
frontrow-entities-0.0.1.1: configure (test)
parent-mailer-0.1.0.0: configure
frontrow-entities-0.0.1.1: build (test)
parent-mailer-0.1.0.0: build
mixpanel-updater-0.1.0.0: install
school-0.0.0: configure
school-0.0.0: build
parent-mailer-0.1.0.0: install
school-lead-mailer-0.1.0.0: configure
school-lead-mailer-0.1.0.0: build
fr-0.1.0.0: install
frontrow-entities-0.0.1.1: Test running disabled by --no-run-tests flag.
fr-0.1.0.0: configure (test)
teacher-mailer-0.1.0.0: configure
fr-0.1.0.0: build (test)
teacher-mailer-0.1.0.0: build
school-lead-mailer-0.1.0.0: install
transfer-mailer-0.1.0.0: configure
transfer-mailer-0.1.0.0: build
parent-0.0.0: install
transfer-mailer-0.1.0.0: install
fr-0.1.0.0: Test running disabled by --no-run-tests flag.
parent-0.0.0: configure (test)
parent-0.0.0: build (test)
teacher-mailer-0.1.0.0: install
fancy-api-0.0.1: install
parent-0.0.0: Test running disabled by --no-run-tests flag.
fancy-api-0.0.1: configure (test)
fancy-api-0.0.1: build (test)
school-0.0.0: install
fancy-api-0.0.1: Test running disabled by --no-run-tests flag.
school-0.0.0: configure (test)
school-0.0.0: build (test)
school-0.0.0: Test running disabled by --no-run-tests flag.
teacher-mailer-0.1.0.0: configure (test)
teacher-mailer-0.1.0.0: build (test)
teacher-mailer-0.1.0.0: Test running disabled by --no-run-tests flag.
transfer-mailer-0.1.0.0: configure (test)
transfer-mailer-0.1.0.0: build (test)
transfer-mailer-0.1.0.0: Test running disabled by --no-run-tests flag.
Completed all 17 actions.

Here's the verbose output for a build that does nothing then a build after a trivial change to one of the packages:

$ stack build --test --pedantic --no-run-tests --verbose 2> /tmp/t1       
Completed all 7 actions.
$ echo >> school/Foundation.hs                                            
$ stack build --test --pedantic --no-run-tests --verbose 2> /tmp/t2
Completed all 8 actions.

You can find /tmp/t[12] here: https://gist.github.com/meteficha/7fba50a7a002daba2db0.

I don't know if this is relevant, but we're using a custom snapshot instead of Stackage.

@snoyberg
Copy link
Contributor

snoyberg commented Sep 8, 2015

I'm fairly certain this is a manifestation of #838.

@meteficha
Copy link
Member Author

I'm not sure if it's the same as #838 because the duplicates occur even with a clean slate. They seem to be part of stack's plan.

@borsboom borsboom added this to the P1: Must milestone Sep 12, 2015
@snoyberg
Copy link
Contributor

@meteficha Maybe I misread your initial report. If the problem is that package "foo" is first built, and then again built for testing, that's everything working as intended. We do it that way to address the cyclical dependency case. There are a number of tricks employed by stack to make the amount of recompilation minimal (though see #834 for more information on future improvements).

@meteficha
Copy link
Member Author

I think that you understood correctly the issue now. But the thing is, this happens only with stack-0.1.4.0 onwards. Below are some example outputs from the CI builds.

So the questions in my head are:

  • Why does stack-0.1.4.0 do a lot more work when stack-0.1.3.1 didn't?
  • Can we get the old behavior back? I assume that the new behavior is fixing some issue, but for us we didn't have any problems with the builds stack-0.1.3.1 did.

I'd like to stress something that perhaps may give you a clue: most packages below are leaves, meaning that no other package depends on them. For example, no one depends on the fancy-api package, while most packages depend on frontrow-entities. On the stack-0.1.3.1 build plan frontrow-entities gets built twice, but fancy-api gets built only once (in test mode). On the stack-0.1.4.1 build plan, both get rebuilt.

I don't know enough about the inner workings of the Cabal library to understand how #834 could solve this issue :).

Using stack-0.1.4.0:

Progress: 0/17              frontrow-entities-0.0.1.1: configure
Progress: 0/17              frontrow-entities-0.0.1.1: build
Progress: 0/17              frontrow-entities-0.0.1.1: install
Progress: 1/17              fancy-api-0.0.1: configure
Progress: 1/17              fancy-api-0.0.1: build
Progress: 1/17              fr-0.1.0.0: configure
Progress: 1/17              fr-0.1.0.0: build
Progress: 1/17              fr-0.1.0.0: install
Progress: 2/17              fr-0.1.0.0: configure (test)
Progress: 2/17              fr-0.1.0.0: build (test)
Progress: 2/17              fr-0.1.0.0: test (suite: fr-test)
Progress: 3/17              frontrow-entities-0.0.1.1: configure (test)
Progress: 3/17              frontrow-entities-0.0.1.1: build (test)
Progress: 3/17              frontrow-entities-0.0.1.1: test (suite: test)
Progress: 4/17              mixpanel-updater-0.1.0.0: configure
Progress: 4/17              mixpanel-updater-0.1.0.0: build
Progress: 4/17              mixpanel-updater-0.1.0.0: install
Progress: 5/17              parent-0.0.0: configure
Progress: 5/17              parent-0.0.0: build
Progress: 5/17              fancy-api-0.0.1: install
Progress: 6/17              fancy-api-0.0.1: configure (test)
Progress: 6/17              fancy-api-0.0.1: build (test)
Progress: 6/17              parent-0.0.0: install
Progress: 7/17              fancy-api-0.0.1: test (suite: test)
Progress: 8/17              parent-0.0.0: configure (test)
Progress: 8/17              parent-mailer-0.1.0.0: configure
Progress: 8/17              parent-0.0.0: build (test)
Progress: 8/17              parent-mailer-0.1.0.0: build
Progress: 8/17              parent-mailer-0.1.0.0: install
Progress: 9/17              school-0.0.0: configure
Progress: 9/17              school-0.0.0: build
Progress: 9/17              parent-0.0.0: test (suite: test)
Progress: 10/17               school-lead-mailer-0.1.0.0: configure
Progress: 10/17               school-lead-mailer-0.1.0.0: build
Progress: 10/17               school-lead-mailer-0.1.0.0: install
Progress: 11/17               teacher-mailer-0.1.0.0: configure
Progress: 11/17               teacher-mailer-0.1.0.0: build
Progress: 11/17               teacher-mailer-0.1.0.0: install
Progress: 12/17               teacher-mailer-0.1.0.0: configure (test)
Progress: 12/17               teacher-mailer-0.1.0.0: build (test)
Progress: 12/17               school-0.0.0: install
Progress: 13/17               teacher-mailer-0.1.0.0: test (suite: test)
Progress: 14/17               school-0.0.0: configure (test)
Progress: 14/17               transfer-mailer-0.1.0.0: configure
Progress: 14/17               school-0.0.0: build (test)
Progress: 14/17               transfer-mailer-0.1.0.0: build
Progress: 14/17               transfer-mailer-0.1.0.0: install
Progress: 15/17               school-0.0.0: test (suite: test)
Progress: 16/17               transfer-mailer-0.1.0.0: configure (test)
Progress: 16/17               transfer-mailer-0.1.0.0: build (test)
Progress: 16/17               transfer-mailer-0.1.0.0: test (suite: test)
Progress: 16/17 Completed all 17 actions. 

Using stack-0.1.3.1:

NOTE: the test command is functionally equivalent to 'build --test'
Progress: 0/12              frontrow-entities-0.0.1.1: configure
Progress: 0/12              frontrow-entities-0.0.1.1: build
Progress: 0/12              yesod-routes-flow-1.0.2: configure
Progress: 0/12              yesod-routes-flow-1.0.2: build
Progress: 0/12              yesod-routes-flow-1.0.2: install
Progress: 1/12              frontrow-entities-0.0.1.1: install
Progress: 2/12              fancy-api-0.0.1: configure (test)
Progress: 2/12              fancy-api-0.0.1: build (test)
Progress: 2/12              fancy-api-0.0.1: test (suite: test)
Progress: 3/12              fr-0.1.0.0: configure (test)
Progress: 3/12              fr-0.1.0.0: build (test)
Progress: 3/12              fr-0.1.0.0: test (suite: fr-test)
Progress: 4/12              frontrow-entities-0.0.1.1: configure (test)
Progress: 4/12              mixpanel-updater-0.1.0.0: configure
Progress: 4/12              frontrow-entities-0.0.1.1: build (test)
Progress: 4/12              mixpanel-updater-0.1.0.0: build
Progress: 4/12              mixpanel-updater-0.1.0.0: install
Progress: 5/12              frontrow-entities-0.0.1.1: test (suite: test)
Progress: 6/12              parent-0.0.0: configure (test)
Progress: 6/12              parent-mailer-0.1.0.0: configure
Progress: 6/12              parent-0.0.0: build (test)
Progress: 6/12              parent-mailer-0.1.0.0: build
Progress: 6/12              parent-mailer-0.1.0.0: install
Progress: 7/12              parent-0.0.0: test (suite: test)
Progress: 8/12              school-0.0.0: configure (test)
Progress: 8/12              school-lead-mailer-0.1.0.0: configure
Progress: 8/12              school-0.0.0: build (test)
Progress: 8/12              school-lead-mailer-0.1.0.0: build
Progress: 8/12              school-lead-mailer-0.1.0.0: install
Progress: 9/12              school-0.0.0: test (suite: test)
Progress: 10/12               teacher-mailer-0.1.0.0: configure (test)
Progress: 10/12               teacher-mailer-0.1.0.0: build (test)
Progress: 10/12               teacher-mailer-0.1.0.0: test (suite: test)
Progress: 11/12               transfer-mailer-0.1.0.0: configure (test)
Progress: 11/12               transfer-mailer-0.1.0.0: build (test)
Progress: 11/12               transfer-mailer-0.1.0.0: test (suite: test)
Progress: 11/12 Completed all 12 actions. 

@snoyberg
Copy link
Contributor

Yes, there was a change: we build all executables and libraries even if a package is a leaf for testing. There are multiple reasons this was desirable, simplest of which is: sometimes test suites need to use the generated executables.

I think the behavior here is correct. Are you reporting that there's been a significant performance impact in aggregate? The progress numbers aren't enough to tell me that there's really a problem.

@meteficha
Copy link
Member Author

It does cause an issue, especially when developing (not so much on CI). For example, if I change something on the test suite, now I have to wait for the whole package to be built twice from scratch, plus the whole test suite, while in theory only a couple of modules from the test suite really need to be rebuilt.

@snoyberg
Copy link
Contributor

That shouldn't be happening, and hasn't been my experience. Do you have a minimal example of that happening?

Also, can you check if this is happening with master?

@meteficha
Copy link
Member Author

I'll rebuild my stack from master and report back.

@meteficha
Copy link
Member Author

On a simple test stack from master went through trying to build the code twice, but didn't really build it. While it wasn't as fast as I'd like (it took 28 seconds after echo >> on a test module), it wasn't too bad. I'll keep using it and yell if something happens.

Regarding the reason why things get built twice, is there a longer-term issue I can subscribe to for working so that these rebuilds aren't necessary anymore? In my own experience I've never needed to use an executable from a test suite, so I'll project this onto others and say that many people feel the same way and are wasting their times waiting for needless recompilations :). If I didn't use Cabal's test-suite and instead had a new fancy-api-test package with the test suite as an executable, I wouldn't need to waste my time waiting for compilations to happen twice. So when a hack gets more efficient than the official way, something seems amiss.

@snoyberg
Copy link
Contributor

I think there are three potentially different problems here:

  1. stack is building the executables when you don't want it to. You can be very explicit about which components you want built with things like stack build package:test-name package2:test2
  2. stack is rebuilding the library for both the test and non-test. In reality, this should be a no-op to Cabal, since the library requires no rebuilding. (This is non-obvious; there was a lot of ugly work with cabal_macros.h to make this a reality.)
  3. There's a cost every time we call into Setup.hs. Not much we can do about that.

The components issue I linked to is about getting more control with Cabal to solve (2), and say "I don't want you to build the library, really, trust me, just build these components I'm telling you about now."

@meteficha
Copy link
Member Author

Using fancy-api:test as a target brings down the noop time I've mentioned to 13s, so it's a nice trick I didn't know about. Unfortunately, it doesn't seem to work from a "cold build" as it tries to install an executable that doesn't exist:

$ stack clean
$ stack test --pedantic --ghc-options=-O0 fancy-api:test --no-run-tests
fancy-api-0.0.1: unregistering (missing dependencies: frontrow-entities)
frontrow-entities-0.0.1.1: unregistering (local file changes)
frontrow-entities-0.0.1.1: configure
frontrow-entities-0.0.1.1: build
frontrow-entities-0.0.1.1: install
fancy-api-0.0.1: build
Preprocessing library fancy-api-0.0.1...
[ 1 of 71] ...
[71 of 71] Compiling Application      ( Application.hs, .stack-work/dist/x86_64-linux/Cabal-1.22.4.0/build/Application.o )
In-place registering fancy-api-0.0.1...
fancy-api-0.0.1: install
Installing library in
/home/felipe/Trabalho/frontrowed/backend/.stack-work/install/x86_64-linux/custom-frontrowed-2.2/7.10.2/lib/x86_64-linux-ghc-7.10.2/fancy-api-0.0.1-HIcNH4tki5aGSt70DpHkYk
Installing executable(s) in
/home/felipe/Trabalho/frontrowed/backend/.stack-work/install/x86_64-linux/custom-frontrowed-2.2/7.10.2/bin
setup-Simple-Cabal-1.22.4.0-x86_64-linux-ghc-7.10.2:
.stack-work/dist/x86_64-linux/Cabal-1.22.4.0/build/fancy-api/fancy-api: does
not exist    
Progress: 2/3
--  While building package fancy-api-0.0.1 using:
      /home/felipe/.stack/setup-exe-cache/setup-Simple-Cabal-1.22.4.0-x86_64-linux-ghc-7.10.2 --builddir=.stack-work/dist/x86_64-linux/Cabal-1.22.4.0/ install
    Process exited with code: ExitFailure 1

@snoyberg
Copy link
Contributor

Yup, you're hitting: haskell/cabal#2780

On Mon, Sep 21, 2015 at 8:11 PM, Felipe Lessa notifications@github.com
wrote:

Using fancy-api:test as a target brings down the noop time I've mentioned
to 13s, so it's a nice trick I didn't know about. Unfortunately, it doesn't
seem to work from a "cold build" as it tries to install an executable that
doesn't exist:

$ stack clean
$ stack test --pedantic --ghc-options=-O0 fancy-api:test --no-run-tests
fancy-api-0.0.1: unregistering (missing dependencies: frontrow-entities)
frontrow-entities-0.0.1.1: unregistering (local file changes)
frontrow-entities-0.0.1.1: configure
frontrow-entities-0.0.1.1: build
frontrow-entities-0.0.1.1: install
fancy-api-0.0.1: build
Preprocessing library fancy-api-0.0.1...
[ 1 of 71] ...
[71 of 71] Compiling Application ( Application.hs, .stack-work/dist/x86_64-linux/Cabal-1.22.4.0/build/Application.o )
In-place registering fancy-api-0.0.1...
fancy-api-0.0.1: install
Installing library in
/home/felipe/Trabalho/frontrowed/backend/.stack-work/install/x86_64-linux/custom-frontrowed-2.2/7.10.2/lib/x86_64-linux-ghc-7.10.2/fancy-api-0.0.1-HIcNH4tki5aGSt70DpHkYk
Installing executable(s) in
/home/felipe/Trabalho/frontrowed/backend/.stack-work/install/x86_64-linux/custom-frontrowed-2.2/7.10.2/bin
setup-Simple-Cabal-1.22.4.0-x86_64-linux-ghc-7.10.2:
.stack-work/dist/x86_64-linux/Cabal-1.22.4.0/build/fancy-api/fancy-api: does
not exist
Progress: 2/3
-- While building package fancy-api-0.0.1 using:
/home/felipe/.stack/setup-exe-cache/setup-Simple-Cabal-1.22.4.0-x86_64-linux-ghc-7.10.2 --builddir=.stack-work/dist/x86_64-linux/Cabal-1.22.4.0/ install
Process exited with code: ExitFailure 1


Reply to this email directly or view it on GitHub
#952 (comment)
.

@meteficha
Copy link
Member Author

Thanks, subscribed. I'll re-open this issue if some new related strangeness happens.

@meteficha
Copy link
Member Author

PS: Here's a formal reference so GitHub links these issues together: haskell/cabal#2780 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants