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

[nix-local-build] new-run, new-test, new-bench, new-exec #3638

Closed
acfoltzer opened this issue Jul 28, 2016 · 18 comments · Fixed by #4722
Closed

[nix-local-build] new-run, new-test, new-bench, new-exec #3638

acfoltzer opened this issue Jul 28, 2016 · 18 comments · Fixed by #4722

Comments

@acfoltzer
Copy link
Collaborator

acfoltzer commented Jul 28, 2016

Summary by @ezyang. These should be easy to do, it's a copy-paste job of the existing run/test/bench code on top of invoking the new-build logic to build things.


These are very important quality-of-life features that make it impractical to use new-build for some of our larger projects. In general, we'd be happy with straightforward ports of the legacy commands, but a couple specific comments arose.

Avoiding unnecessary work

This is a lot more important when working with metapackages, since we don't want to have to build every test suite in the metapackage in order to build one particular test. The way the [COMPONENTS] argument currently works for new-build should be fine for this.

Easier argument passing for test and bench

cabal run [FLAGS] [EXECUTABLE] [-- EXECUTABLE_FLAGS] makes it easy to pass arguments to the executable by just putting them after the --. It would be nice to have the same for test and bench, rather than having to use --test-options or --benchmark-options flags.

@ezyang
Copy link
Contributor

ezyang commented Jul 28, 2016

So, I guess, if you say cabal new-test test1 test2 -- --some-test-option this test option would be passed to both tests you are running?

@ezyang
Copy link
Contributor

ezyang commented Aug 9, 2016

Note that however this gets implemented, make sure it works with --enable-coverage, c.f. #3682

@elliottt
Copy link

elliottt commented Sep 3, 2016

Would it be possible to add new-exec to this list? I prefer using cabal exec ghci to using cabal repl as it doesn't try to load any modules.

@ezyang ezyang added this to the 2.0 milestone Sep 8, 2016
@ezyang ezyang changed the title [nix-local-build] new-run, new-test, new-bench [nix-local-build] new-run, new-test, new-bench, new-exec Sep 8, 2016
@dcoutts
Copy link
Contributor

dcoutts commented Sep 8, 2016

@elliottt on that topic, we should decide if that would be the better behaviour anyway and/or add a flag to repl to control it. I'm not sure if there's a ticket for it, but if not please open one and explain the advantages/disadvantages.

@elliottt
Copy link

@dcoutts a quick search turned up #2592 that does a fairly good job of framing the problem. My use case is similar: large projects with lots of modules make cabal repl unpleasant, as it doesn't allow me to start up the repl and then load only the modules I'm interested in.

@ezyang
Copy link
Contributor

ezyang commented Sep 21, 2016

I spent a little time looking at how to implement new-run. There does seem to be some rejiggering needed:

  1. We need a way of specifying a specific executable as the one we want to run. Not easily done because this is a callback from pre-build. Maybe we should do it repl style by marking the ElaboratedConfiguredPackage according to the command line (much how we mark the things we want to repl in a similar way.) But the way repl is run is bogus, see cabal new-repl target1 target2 shouldn't bring up GHCi prompt twice #3659, so maybe it should be done properly here. CC @dcoutts
  2. Make sure you look at Distribution/Client/Run.hs, it's accumulated a bunch of edge case handling which should be preserved
  3. Need a mode which assumes that the project is up to date and directly invokes the executable with as little overhead as possible. But do we still have to run the pre-build phase? It would definitely be convenient to do so, because that's the easiest way to read off the dist-dir. We could read out the cached value w/o checking if it's invalid? Or maybe we can figure out executable location from first principles?

@dcoutts
Copy link
Contributor

dcoutts commented Sep 22, 2016

@ezyang I don't understand why it's so difficult. Isn't it just a matter of doing what new-build does for an exe target, ie making sure it's up-to-date, and then executing it?

Sure we have to check that the target the user has given does correspond to an exe. Not so hard. Then that gives us an elaborated plan that's up to date for the exe, and we can read off from that where the exe lives in the file system, then we just run it.

If a "no rebuild" mode is needed, then we just make sure the plan is up to date, read off the location of the executable from the elaborated plan, checking the exe exists, and do the same invocation.

And yes, the logic for invoking needs to be ported from Run.hs.

What am I missing?

@dcoutts
Copy link
Contributor

dcoutts commented Sep 22, 2016

Ok, so the runProjectPreBuildPhase should return the resolved targets selected. That'd be most direct. I'm doing some refactoring in ProjectOrchestration so I can do this.

@phadej
Copy link
Collaborator

phadej commented Jan 28, 2017

FWIW, there is bare bones new-test which simply builds and runs all tests matching build targets.

@23Skidoo 23Skidoo modified the milestones: 2.0.1, 2.0 Feb 17, 2017
@acfoltzer
Copy link
Collaborator Author

#4304 has new-exec

@acfoltzer
Copy link
Collaborator Author

Per @dmwit, new-test is also now implemented in master

@hvr
Copy link
Member

hvr commented Mar 15, 2017

@dcoutts @ezyang @dmwit I notice with #4179 merged, there's now cabal new-run (courtesy of f2a8bdf), but as the commit message says:

So far they just build the exe bench and don't do the next steps, but they do at least select the right components to build.

So, this sounds like we're almost there? is the only thing left to do actually invoke the produced executable component now?

@dcoutts
Copy link
Contributor

dcoutts commented Mar 16, 2017

@hvr yes, new-run and new-bench CLI bits are in but they only build the things, they don't yet run.

@ezyang
Copy link
Contributor

ezyang commented Apr 30, 2017

Let's track the new-run business at #4477

@23Skidoo
Copy link
Member

So, I guess, if you say cabal new-test test1 test2 -- --some-test-option this test option would be passed to both tests you are running?

I should note that this syntax is ambiguous: does test2 name a target, or is it the beginning of the argument list for test1?

@fgaz
Copy link
Member

fgaz commented Jul 29, 2017

Tracking the args issue at #4643

@hvr
Copy link
Member

hvr commented Jul 30, 2017

I should note that this syntax is ambiguous: does test2 name a target, or is it the beginning of the argument list for test1?

IMHO, everything left of -- is either a cabal-flag or an argument (i.e. the test-component names). While everything right of -- is to be treated as argument to the executed component (iirc this nicely works for +RTS too). This is how I interpret the original description of this ticket regarding easy parsing.

To keep things simple (and avoid clever heuristics), this would imply that the use of -- is mandatory as soon as you want to pass unquoted flags to an test-suite (w/o --test-options).

Some examples to clarify what I mean:

  • cabal new-test test1 test2 -- --some-test-option: two testsuites test1 & test2; and one flag passed to both
  • cabal new-test test1 test2: two testsuites test1 & test2
  • cabal new-test test1 test2 --some-test-option: invalid, as --some-test-option would be assumed to be a cabal flag.
  • cabal new-test test1 -- test2 --some-test-option: one testsuite test1, which is invoked with two arguments test2 --some-test-option.

We can still support per-test-component flags via --test-options flags, maybe

  • cabal new-test test1 --test-options 'flags for test1' test2 --test-options 'flags for test2'

@23Skidoo
Copy link
Member

Our cmdline parser currently doesn't provide the information about the relative position of -- in the extra args list, in both case 1 and case 4 above the client will receive ["test1", "test2", "--some-test-option"] as input.

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

Successfully merging a pull request may close this issue.

8 participants