Skip to content

Automatically configure and build before 'cabal test' and 'cabal bench' #995

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

Closed
wants to merge 1 commit into from

Conversation

ttuegel
Copy link
Member

@ttuegel ttuegel commented Aug 9, 2012

Issue: #585

I can merge this pull request myself, but I wanted to solicit feedback from interested parties first (especially because of the command line UI change).

This patch changes the behaviour of the test and bench commands in cabal-install so that the package in the current directory is configured and built before attempting to run the test suites or benchmarks. The net result is that running 'cabal unpack foo; cd foo; cabal test' does what you mean, rather than ordering you to run 'cabal configure' first. Happily, this also solves the problem where a developer's changes were not being detected in the test suite because the affected files had not been rebuilt.

This is accompanied by a user interface change: the test and bench commands now accept ConfigFlags and ConfigExFlags in the same way the install command does. I think the test and bench commands should reflect the install command because they now do mostly the same things. Also, if we plan to have the test and bench commands eventually pull in the necessary dependencies, there will really be no way around this UI change.

@tibbe
Copy link
Member

tibbe commented Aug 9, 2012

Could you please rebase your commits so you don't have those revert commits and the merge commit? That would make it easier to review the changes.

If I understood your explanation of the UI changes correctly, cabal test and cabal bench now accept the same flags as cabal install. Do all the flags from cabal install make sense even if we're not installing dependencies? Long term we will most likely install dependencies and this becomes a moot point, but in the meantime I'd like to not confuse users by presenting them with flags that in fact have no effect.

@23Skidoo
Copy link
Member

23Skidoo commented Aug 9, 2012

How difficult would it be to apply the same change to cabal build?

The test and benchmark cabal-install commands now accept ConfigFlags and
ConfigExFlags in the same way as the install command, so that the
package can be configured before running test suites or benchmarks. Also
like the install command, the test and bench commands run the build
command with the default options to perform any necessary rebuilds.
@tibbe
Copy link
Member

tibbe commented Aug 9, 2012

@23Skidoo I believe build already runs configure, if needed. Isn't it so @ttuegel. I think there's annoying case where build wont imply configure, unless the user has run configure once manually. I think we should get rid of that annoying behavior.

@23Skidoo
Copy link
Member

23Skidoo commented Aug 9, 2012

@tibbe

I think there's annoying case where build wont imply configure, unless the user has run configure once manually

That's what I meant - changing build to take the same flags as configure so that the user would never have to run configure separately at all.

@23Skidoo
Copy link
Member

23Skidoo commented Aug 9, 2012

@tibbe

If I understood your explanation of the UI changes correctly, cabal test and cabal bench now accept the same flags as cabal install. Do all the flags from cabal install make sense even if we're not installing dependencies?

From the code, I see that test and bench were changed to accept ConfigFlags and ConfigExFlags, which are the same flags that cabal config accepts. installCommand additionally accepts InstallFlags and HaddockFlags.

@ttuegel
Copy link
Member Author

ttuegel commented Aug 9, 2012

@tibbe Thanks for recommending the rebase. I'm still learning git, and my brain has been stuck in darcs-mode recently :)

As @23Skidoo pointed out, the test and bench commands don't accept Haddock flags, but they accept the other (extra) flags that install does, i.e., they do not accept any InstallFlags. As you pointed out, some of the ConfigExFlags may not have any effect until we start to pull in dependencies, such as --constraint and --preference, but they should affect package visibility to the compiler.

@ttuegel
Copy link
Member Author

ttuegel commented Aug 9, 2012

cabal build will not run configure unless the package has already been configured. It is just a wrapper around the build command defined by the Cabal library, so it will only reconfigure when ./Setup build would reconfigure. It would be trivial to change the cabal-install build command to follow the semantics this pull request implements for test and bench. Is that the desired behaviour?

@tibbe
Copy link
Member

tibbe commented Aug 9, 2012

Here's what I think:

  1. build should be changed to always run configure first, regardless if we add support for passing through configure flags. It's confusing today that build will sometimes imply configure and sometimes not.
  2. test and bench should imply configure and build.
  3. I think we should make build, test, and bench all take the configure flags so no one (except perhaps distro packagers or such) needs to run configure again. I suggest you send out and email to [email protected] suggesting this change. Just to make sure that there isn't a use case (e.g. distro packaging) that we make impossible by having e.g. build imply configure. As long as we leave the option of manually running configure (or build before test/bench) I think we should be fine.

@tibbe
Copy link
Member

tibbe commented Aug 10, 2012

I chatted a bit with @dcoutts on IRC today. We don't need to be worried about distros as long as we're not touching Setup.hs. build should definitely imply configure and test/bench build (i.e. we should do 1 & 2 right now). We're still discussing the configure flags as out opinions differ a bit there.

If you want you can prepare a patch that does what your current patch does minus passing configure flags from test/bench to configure (i.e. configure would reuse the flags from the last configure run) and I can apply it right now. We can separate the configure flags issue into a separate issue and make some progress that way.

@tibbe
Copy link
Member

tibbe commented Aug 12, 2012

I'm closing this request for now. Most of the changes are in #997 and there are some issues we need to resolve with @dcoutts before the remaining changes (i.e. pass through configure flags) can go in.

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

Successfully merging this pull request may close these issues.

3 participants