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

Introduce `stack run` command line option #3952

Merged
merged 14 commits into from Jun 24, 2018

Conversation

@rszibele
Copy link
Contributor

@rszibele rszibele commented Apr 1, 2018

This pull request introduces stack run which allows directly building and running a specified executable similar to cabal run. If no executable is specified, it defaults to the first available executable in the project. This implements the whishlist issue: #233

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md

I tested it by running the newly built stack through stack:

  • stack exec stack run
  • stack exec -- stack run help
  • stack exec -- stack run -- help

I also tested it with a simple project which prints out the command line arguments:

  • stack run -> []
  • stack run "Hello, world!" -> ["Hello world"]
  • stack run "Hello" "World" -> ["Hello", "World"]
  • stack run -- "Hello" "World" -> ["Hello", "World"]
  • stack run hello-exe "Hello World" -> ["Hello world"]
  • stack run hello-exe "Hello" "World" -> ["Hello", "World"]
  • stack run -- hello-exe "Hello" "World" -> ["Hello", "World"]
    After removing all executables from the project:
  • stack run -> No executables found.

Any thoughts on the code? improvements? I'm pretty new to Haskell and stack.

rszibele added 3 commits Apr 1, 2018
@AshleyYakeley
Copy link

@AshleyYakeley AshleyYakeley commented Apr 15, 2018

Does stack run build the executable first?

@rszibele
Copy link
Contributor Author

@rszibele rszibele commented Apr 15, 2018

@AshleyYakeley No, just like with stack exec the stack run command requires the project to already be built.

@AshleyYakeley
Copy link

@AshleyYakeley AshleyYakeley commented Apr 15, 2018

OK, so this does not implement #233, which is for a stack run command that builds and then runs.

@rszibele
Copy link
Contributor Author

@rszibele rszibele commented Apr 15, 2018

@AshleyYakeley good catch, the original issue was with a build before execution. I went off @johnynek suggestion with running the first available executable without building like stack exec.

I'll be implementing that after my exams are over.

@AshleyYakeley
Copy link

@AshleyYakeley AshleyYakeley commented Apr 15, 2018

OK, so will it build the entire project, or just enough to build the executable in question?

@rszibele
Copy link
Contributor Author

@rszibele rszibele commented Apr 16, 2018

I'm tending towards only building the package which contains the executable. Given that the dependencies are properly defined, it shouldn't be required to build all packages, at least that's my line of thinking. Thoughts?

@AshleyYakeley
Copy link

@AshleyYakeley AshleyYakeley commented Apr 16, 2018

I think that's sensible.

@rszibele
Copy link
Contributor Author

@rszibele rszibele commented Jun 4, 2018

stack run now also builds the executable prior to running it, like directly running stack build :myexecutable. It now also supports specifying an executable as the first command and if it is omitted, it will default to the first available executable in the project.

I have edited my pull request to reflect these changes.

Copy link
Contributor

@snoyberg snoyberg left a comment

Thanks for the PR! I've left some comments below, let me know if you have any questions.

getRunCmd args = do
pkgComponents <- liftM (map lpvComponents . Map.elems . lpProject) getLocalPackages
let executables = filter isCExe $ concatMap Set.toList pkgComponents
let argExe = if not (null args) then find (\x -> x == (CExe $ T.pack $ head args)) executables

This comment has been minimized.

@snoyberg

snoyberg Jun 15, 2018
Contributor

I realize that the not (null args) ensures that head args is total. However, I still prefer to avoid partial function calls, since they can easily turn into bugs with a refactor. Would it be possible to replace this with a pattern match, e.g.:

case args of
  [] -> Nothing
  firstArg:_ -> ...
let executables = filter isCExe $ concatMap Set.toList pkgComponents
let argExe = if not (null args) then find (\x -> x == (CExe $ T.pack $ head args)) executables
else Nothing
let firstExe = if not (null executables) then Just $ head executables else Nothing

This comment has been minimized.

@snoyberg

snoyberg Jun 15, 2018
Contributor

Same applies here, though I think you can also use listToMaybe.

let argExe = if not (null args) then find (\x -> x == (CExe $ T.pack $ head args)) executables
else Nothing
let firstExe = if not (null executables) then Just $ head executables else Nothing
let (exe, args') = if isJust argExe then (argExe, tail args) else (firstExe, args)

This comment has been minimized.

@snoyberg

snoyberg Jun 15, 2018
Contributor

And this could be combined with the pattern match in the above let binding to avoid the partial tail call.

let (exe, args') = if isJust argExe then (argExe, tail args) else (firstExe, args)
case exe of
Just (CExe exe') -> do
Stack.Build.build (const (return ())) Nothing defaultBuildOptsCLI{boptsCLITargets = [T.concat [T.pack ":", exe']]}

This comment has been minimized.

@snoyberg

snoyberg Jun 15, 2018
Contributor

Slightly more idiomatic may be: T.cons ':' exe'.

Stack.Build.build (const (return ())) Nothing defaultBuildOptsCLI{boptsCLITargets = [T.concat [T.pack ":", exe']]}
return (T.unpack exe', args')
_ -> liftIO $ do
hPutStrLn stderr "No executables found."

This comment has been minimized.

@snoyberg

snoyberg Jun 15, 2018
Contributor

This should probably use logError instead, which would need to be outside of liftIO.

@@ -786,6 +792,7 @@ execCmd ExecOpts {..} go@GlobalOpts{..} =
ExecOptsPlain -> do
(cmd, args) <- case (eoCmd, eoArgs) of
(ExecCmd cmd, args) -> return (cmd, args)
(ExecRun, args) -> return ("", args)

This comment has been minimized.

@snoyberg

snoyberg Jun 15, 2018
Contributor

Can you explain why we need an empty command case here? Is this a code path which shouldn't be triggered? If so, I would rather generate an error message here, since the exec call below will fail anyway.

@rszibele
Copy link
Contributor Author

@rszibele rszibele commented Jun 23, 2018

Hello @snoyberg, thank you for your code review, it has been very insightful. I have updated my pull request according to your suggestions. I have also added docker support, as it was the code path (ExecOptsPlain) which I mistakenly left out during the implementation.

I have re-tested everything and also tested it with docker enabled.

Copy link
Contributor

@snoyberg snoyberg left a comment

LGTM!

@snoyberg snoyberg merged commit e727913 into commercialhaskell:master Jun 24, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Wizek
Copy link

@Wizek Wizek commented Jun 24, 2018

🎉 Hooray! Thanks guys!

Atry added a commit to Atry/Haskelly that referenced this pull request Feb 7, 2019
…askelly-dev#84)

`stack run` is now a built-in command (see commercialhaskell/stack#3952)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.