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

Enable tests and benchmarks for stack #92

Closed
wants to merge 2 commits into from

Conversation

jneira
Copy link
Collaborator

@jneira jneira commented Jan 17, 2020

  • In hie we detected that, if you did not a stack build --test we were not able to load the ghc flags using cabal-helper+hie-bios
  • This pr adds --test --benchmarks to all stack stages triggered by cabal-helper to ensure all components are enabled (with --no-run.testsand no-run-benchamrks to avoid unwanted work)
  • I'v tested manually that the error is not hrowed with this version

@jneira jneira requested a review from DanielG January 17, 2020 13:55
@jneira jneira changed the title Enable tests and benchmarks for stack [WIP] Enable tests and benchmarks for stack Jan 18, 2020
@jneira
Copy link
Collaborator Author

jneira commented Jan 18, 2020

I am not able to reproduce haskell/haskell-ide-engine#1564 in my windows 10 without this patch. @fendor do you have a reproducible case to test this against?

Copy link
Owner

@DanielG DanielG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two objections here:

  1. No regression test

The ghc-session suite already has a test case with a test and benchmark (exelib) but we just build the components we do detect, so you'd have to add support for specifying which components should be there in the test config.

  1. Performance

I'm concerned that by unconditionally enabling tests and benchmarks for c-h invocations of stack build we'll force the user's stack build calls to reconfigure the packages without those components again. Which gets us into a sort of ping-pong situation which I'd like to avoid.

I'm not sure what stack's mechanism for avoiding that is, in principle it could be looking at the existing LocalBuildInfo and figure out what to build based on that, that's what v1-build did after all which they likely copied. You should test that out.

@@ -518,17 +518,15 @@ buildProjectTarget qe mu stage = do
} -> do
let projdir = plStackProjectDir qeProjLoc
let workdir_opts = Stack.workdirArg qe
let opts = workdir_opts ++
[ "--stack-yaml="++plStackYaml, "build", "."
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, you're changing the behaviour here. That "." is load-bearing :)

Without it Stack build the whole project, even when in a package sub-directory, but with it it will only build the package it's currently in. Hence the two cases below.

Stack docs say this:

Finally: if you provide no targets (e.g., running stack build), stack will implicitly pass in all of your local packages. If you only want to target packages in the current directory or deeper, you can pass in ., e.g. stack build ..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jum, to be honest i didn't spot the difference, good catch

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably add a comment clarifying the situation then :)

@fendor
Copy link
Contributor

fendor commented Jan 18, 2020

@jneira I have a reproducable case, I will try out the PR.

@jneira
Copy link
Collaborator Author

jneira commented Jan 19, 2020

Thanks for the review and sorry if it was a little bit hasty

  1. No regression tests

Yeah, they are needed. In fact i can't reproduce anymore manually the hie error without this so that is the first step.

  1. Performance

... and user experience.

I was wondering if it could be conditioned to some already exixting info. To get the minimum info i suppose we have to build the project with --only-configure to get the setup-info if it is not generated. So i would activate test and benchmarks always if there is no previous build and only activate them afterwards if user is opening a module that belongs to tests or benchmarks.
I am assuming that if users open a test module they can't be surprised if we build the project with tests enabled.

@DanielG
Copy link
Owner

DanielG commented Jan 19, 2020

I was wondering if it could be conditioned to some already exixting info. To get the minimum info i suppose we have to build the project with --only-configure to get the setup-info if it is not generated. So i would activate test and benchmarks always if there is no previous build and only activate them afterwards if user is opening a module that belongs to tests or benchmarks.

Yeah something like that. I'd like to stress again that already just rewriting setup-config is potentially problematic, so I'd like you to test out whether or not stack will pick up the previous enable/disable state for tests and benchmarks to forgoe reconfiguring. You should be able to tell by the mtime on the setup-config file or by looking at verbose build output.

As for detecting if a file belongs to a (currently disabled) test/bench component: I'm pretty sure that's going to be somewhat involved to impossible since we can't rely on the info we usually have for enabled components. So you'd have to build a model of what is in a package just by looking at the (unconfigured) package description. I had a related discussion with someone (@fendor or @bubba probably) a while ago about detecting if a file is part of a project or not regardless of component enablement but this is going a step beyond that even.

We should have a discussion about that seperately it's a pretty tricky topic. My gut feeling is that we'd need changes in Cabal to support this though so beware.

I am assuming that if users open a test module they can't be surprised if we build the project with tests enabled.

I'm not so concerned with a one time delay here, the problem is the ping-pong behaviour. As long as the switch to tests/benchmarks enabled happens once and then sticks, its fine. But if cabal-helper and the user keep fighting over the enable/disable state that's bad :)

@jneira
Copy link
Collaborator Author

jneira commented Feb 10, 2020

One caveat i've think out: test/bench component could be broken (included its dependencies), and configure/build with them active would fail so user may want to no activate them.
So, as discussed, we should only build with them included if the user opens a file that belongs to a test or bench component, and only that component, not all of them.
The problem could be: before make a stack build --only-configure we dont even know which component owns the file. Maybe the config generated configuring it by default (i.e without test and bench active) could have that info? I hope so

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