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

new-run #4586

Merged
merged 26 commits into from Jul 16, 2017

Conversation

Projects
5 participants
@fgaz
Collaborator

fgaz commented Jul 4, 2017

Fixes #4477.

The ci should be green now. Changelog, docs, and more tests incoming.

Can someone review it in the meantime?

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jul 4, 2017

@fgaz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dcoutts to be a potential reviewer.

mention-bot commented Jul 4, 2017

@fgaz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dcoutts to be a potential reviewer.

@fgaz

This comment has been minimized.

Show comment
Hide comment
@fgaz

fgaz Jul 4, 2017

Collaborator

ping @dmwit

Collaborator

fgaz commented Jul 4, 2017

ping @dmwit

@ezyang

Looks OK to me. Would definitely good to expand test coverage.

Show outdated Hide outdated cabal-install/Distribution/Client/CmdRun.hs Outdated
pkg
exe
</> exe
let args = drop 1 targetStrings

This comment has been minimized.

@ezyang

ezyang Jul 4, 2017

Contributor

(A) Then it wouldn't be necessary to subsequently drop it here!

@ezyang

ezyang Jul 4, 2017

Contributor

(A) Then it wouldn't be necessary to subsequently drop it here!

This comment has been minimized.

@fgaz

fgaz Jul 5, 2017

Collaborator

I did this because (see B1 and B2) we don't know anything about the targets at the beginning of the function, and an empty targetStrings could have a meaning. If i wrote a match for (command:args) and [] then I'd have to duplicate most of the code or put everything in an helper function.

the take/drop approach handles well the absence of a target component.

But now that i think of it, what if we do
new-run <no explicit exe here, there is only one in the package> args? the first arg will be interpreted as a target and the command will fail.
But I don't think there is a way to handle this without ambiguities.

@fgaz

fgaz Jul 5, 2017

Collaborator

I did this because (see B1 and B2) we don't know anything about the targets at the beginning of the function, and an empty targetStrings could have a meaning. If i wrote a match for (command:args) and [] then I'd have to duplicate most of the code or put everything in an helper function.

the take/drop approach handles well the absence of a target component.

But now that i think of it, what if we do
new-run <no explicit exe here, there is only one in the package> args? the first arg will be interpreted as a target and the command will fail.
But I don't think there is a way to handle this without ambiguities.

This comment has been minimized.

@dmwit

dmwit Jul 5, 2017

Collaborator

How about new-run -- args for "run whatever executable is the default, with arguments args" and new-run args for "search for an executable named args and run it"? Separating arguments for a subprocess with -- is a pretty common idiom. (e.g. old-style run supports this.)

@dmwit

dmwit Jul 5, 2017

Collaborator

How about new-run -- args for "run whatever executable is the default, with arguments args" and new-run args for "search for an executable named args and run it"? Separating arguments for a subprocess with -- is a pretty common idiom. (e.g. old-style run supports this.)

This comment has been minimized.

@fgaz

fgaz Jul 6, 2017

Collaborator

Good idea.

Cabal's flags are filtered from the targetStrings already, so it should simply be a matter of checking the first targetString

@fgaz

fgaz Jul 6, 2017

Collaborator

Good idea.

Cabal's flags are filtered from the targetStrings already, so it should simply be a matter of checking the first targetString

This comment has been minimized.

@fgaz

fgaz Jul 6, 2017

Collaborator

No it isn't.

The first -- is stripped from the targetStrings as well, probably because it marks the end of cabal flags and the beginning of the targets.

But if we document it appropriately we could use two of them, the first separating flags and targets, the second targets and targets' args

(actually its only use will be as a "placeholder" for the target when args are also needed, like @dmwit says)

@fgaz

fgaz Jul 6, 2017

Collaborator

No it isn't.

The first -- is stripped from the targetStrings as well, probably because it marks the end of cabal flags and the beginning of the targets.

But if we document it appropriately we could use two of them, the first separating flags and targets, the second targets and targets' args

(actually its only use will be as a "placeholder" for the target when args are also needed, like @dmwit says)

This comment has been minimized.

@fgaz

fgaz Jul 7, 2017

Collaborator

Ok, -- -- is ugly and prevents future modification of the syntax, so I'll leave this as it is for now (args only when the exe/package is specified)

@fgaz

fgaz Jul 7, 2017

Collaborator

Ok, -- -- is ugly and prevents future modification of the syntax, so I'll leave this as it is for now (args only when the exe/package is specified)

This comment has been minimized.

@fgaz

fgaz Jul 7, 2017

Collaborator

btw, #3638 may be the origin of -- --

@fgaz

fgaz Jul 7, 2017

Collaborator

btw, #3638 may be the origin of -- --

This comment has been minimized.

@fgaz

fgaz Jul 10, 2017

Collaborator

Apparently even new-run target -exearg --exearg doesn't work, as all --prefixed args are interpreted as cabal args, while new-run target -- --exearg works.

Fixing this requires some modifications to the args parsing, but I don't think this is needed, as this was also the behaviour of old-run (except #4600).

edit: also run <empty> args isn't supported in old-run, so I'll leave new-run as is for now

@fgaz

fgaz Jul 10, 2017

Collaborator

Apparently even new-run target -exearg --exearg doesn't work, as all --prefixed args are interpreted as cabal args, while new-run target -- --exearg works.

Fixing this requires some modifications to the args parsing, but I don't think this is needed, as this was also the behaviour of old-run (except #4600).

edit: also run <empty> args isn't supported in old-run, so I'll leave new-run as is for now

This comment has been minimized.

@dmwit

dmwit Jul 10, 2017

Collaborator

Keep in mind: modeling our behavior after old-style run is good to a point (because it lets us leverage folks' familiarity with the old UI); but as we're making lots of breaking UI changes here, this is also a good time to fix warts. So I think any behavior we choose should either be the right behavior or have a clear forward-compatibility story with the right behavior; backward-compatibility with the old, wrong behavior is less of a concern.

@dmwit

dmwit Jul 10, 2017

Collaborator

Keep in mind: modeling our behavior after old-style run is good to a point (because it lets us leverage folks' familiarity with the old UI); but as we're making lots of breaking UI changes here, this is also a good time to fix warts. So I think any behavior we choose should either be the right behavior or have a clear forward-compatibility story with the right behavior; backward-compatibility with the old, wrong behavior is less of a concern.

- T4477-1.0 (exe:foo) (first run)
Configuring executable 'foo' for T4477-1.0..
Preprocessing executable 'foo' for T4477-1.0..
Building executable 'foo' for T4477-1.0..

This comment has been minimized.

@ezyang

ezyang Jul 4, 2017

Contributor

Maybe not for this PR, but we should think carefully about whether or new-run should output this sort of information. In favor: it's good to be able to see when your executable is being built. Against: you won't be able to use new-run to run executables that you want to run as part of a pipeline, since there will be extra Cabal build progress goo in your stderr. Perhaps this is related to the desire for a mode, "please run this as quickly as possible, don't rebuild at all!"

@ezyang

ezyang Jul 4, 2017

Contributor

Maybe not for this PR, but we should think carefully about whether or new-run should output this sort of information. In favor: it's good to be able to see when your executable is being built. Against: you won't be able to use new-run to run executables that you want to run as part of a pipeline, since there will be extra Cabal build progress goo in your stderr. Perhaps this is related to the desire for a mode, "please run this as quickly as possible, don't rebuild at all!"

This comment has been minimized.

@dmwit

dmwit Jul 5, 2017

Collaborator

The question of whether new-run should output its own information is a good one. My stance on "build or not" is that new-run should always rebuild the appropriate files, and new-exec should never rebuild anything (but should ensure that all the project's executables would be on the PATH if they were built). This makes the divide nice and clean, and supports both needs.

@dmwit

dmwit Jul 5, 2017

Collaborator

The question of whether new-run should output its own information is a good one. My stance on "build or not" is that new-run should always rebuild the appropriate files, and new-exec should never rebuild anything (but should ensure that all the project's executables would be on the PATH if they were built). This makes the divide nice and clean, and supports both needs.

This comment has been minimized.

@fgaz

fgaz Jul 6, 2017

Collaborator

We also have -v0

@fgaz

fgaz Jul 6, 2017

Collaborator

We also have -v0

Show outdated Hide outdated cabal-install/Distribution/Client/CmdRun.hs Outdated
Show outdated Hide outdated cabal-install/Distribution/Client/CmdRun.hs Outdated
-> ElaboratedInstallPlan -- ^ a plan in with to search for matching exes
-> [(ElaboratedConfiguredPackage, UnqualComponentName)] -- ^ the matching package and the exe name
extractMatchingElaboratedConfiguredPackages
pkgId component = nubBy equalPackageIdAndExe

This comment has been minimized.

@ezyang

ezyang Jul 4, 2017

Contributor

Why is a nub necessary here? If the same package and component show up multiple times, isn't that a problem?

@ezyang

ezyang Jul 4, 2017

Contributor

Why is a nub necessary here? If the same package and component show up multiple times, isn't that a problem?

This comment has been minimized.

@fgaz

fgaz Jul 5, 2017

Collaborator

If the there are multiple exes, then for each exe there will be an ElaboratedConfiguredPackage in the ElaboratedInstallPlan.

For each ElaboratedConfiguredPackage then, when elabPkgOrComp (see executablesOfPackage) is ElabComponent then there will be one exe, the matching one (it's a Maybe), but if it's an ElabPackage (which happens in custom builds), then that field is Nothing and we have to look at the executables field, which contains every exe in the package, hence the duplication.

I can't dedup before that point because i need both the package and the component name to do so (what if two packages have the seme exe name?).

Maybe I should add a comment about this. Or maybe it's a bug? Or should I add another field to ElaboratedConfiguredPackage/ElaboratedInstallPlan?

@fgaz

fgaz Jul 5, 2017

Collaborator

If the there are multiple exes, then for each exe there will be an ElaboratedConfiguredPackage in the ElaboratedInstallPlan.

For each ElaboratedConfiguredPackage then, when elabPkgOrComp (see executablesOfPackage) is ElabComponent then there will be one exe, the matching one (it's a Maybe), but if it's an ElabPackage (which happens in custom builds), then that field is Nothing and we have to look at the executables field, which contains every exe in the package, hence the duplication.

I can't dedup before that point because i need both the package and the component name to do so (what if two packages have the seme exe name?).

Maybe I should add a comment about this. Or maybe it's a bug? Or should I add another field to ElaboratedConfiguredPackage/ElaboratedInstallPlan?

@@ -90,7 +121,8 @@ runAction (configFlags, configExFlags, installFlags, haddockFlags)
baseCtx <- establishProjectBaseContext verbosity cliConfig
targetSelectors <- either (reportTargetSelectorProblems verbosity) return
=<< readTargetSelectors (localPackages baseCtx) targetStrings
=<< readTargetSelectors (localPackages baseCtx)

This comment has been minimized.

@ezyang

ezyang Jul 4, 2017

Contributor

It is too bad there is no singular version of readTargetSelectors, since it seems like that is what you want.

@ezyang

ezyang Jul 4, 2017

Contributor

It is too bad there is no singular version of readTargetSelectors, since it seems like that is what you want.

Show outdated Hide outdated cabal-install/Distribution/Client/CmdRun.hs Outdated
Show outdated Hide outdated cabal-install/Distribution/Client/CmdRun.hs Outdated
let args = drop 1 targetStrings
runProgramInvocation
verbosity
(simpleProgramInvocation exePath args)

This comment has been minimized.

@ezyang

ezyang Jul 4, 2017

Contributor

I don't think you want runProgramInvocation here; it doesn't propagate the exit code. Use rawSystemExitWithEnv instead, see Distribution/Client/Run.hs for how it is previously done. You may also want to consider adding LD_LIBRARY_PATH if it is necessary.

@ezyang

ezyang Jul 4, 2017

Contributor

I don't think you want runProgramInvocation here; it doesn't propagate the exit code. Use rawSystemExitWithEnv instead, see Distribution/Client/Run.hs for how it is previously done. You may also want to consider adding LD_LIBRARY_PATH if it is necessary.

This comment has been minimized.

@fgaz

fgaz Jul 5, 2017

Collaborator

Will do.
Isn't LD_LIBRARY_PATH automated by the os or something?

@fgaz

fgaz Jul 5, 2017

Collaborator

Will do.
Isn't LD_LIBRARY_PATH automated by the os or something?

This comment has been minimized.

@fgaz

fgaz Jul 13, 2017

Collaborator

I checked and it does propagate it, as it uses rawSystemExit under the hood.

@fgaz

fgaz Jul 13, 2017

Collaborator

I checked and it does propagate it, as it uses rawSystemExit under the hood.

This comment has been minimized.

@23Skidoo

23Skidoo Jul 14, 2017

Member

Do we have a test case that checks that new-run propagates the exit code?

@23Skidoo

23Skidoo Jul 14, 2017

Member

Do we have a test case that checks that new-run propagates the exit code?

This comment has been minimized.

@fgaz

fgaz Jul 15, 2017

Collaborator

Now we have one

@fgaz

fgaz Jul 15, 2017

Collaborator

Now we have one

@23Skidoo

This comment has been minimized.

Show comment
Hide comment
@23Skidoo

23Skidoo Jul 5, 2017

Member

Revert "(,) is not Traversable on older base versions"

Note that you can also do git rebase -i and just remove that commit from your branch. This is not super important, but makes history cleaner and PRs a bit easier to review.

Member

23Skidoo commented Jul 5, 2017

Revert "(,) is not Traversable on older base versions"

Note that you can also do git rebase -i and just remove that commit from your branch. This is not super important, but makes history cleaner and PRs a bit easier to review.

@23Skidoo 23Skidoo requested a review from dcoutts Jul 5, 2017

@23Skidoo

This comment has been minimized.

Show comment
Hide comment
@23Skidoo

23Skidoo Jul 5, 2017

Member

And by the way, can you please format your commit messages in the standard way? See this article: https://chris.beams.io/posts/git-commit/ for an explanation of what that means. No need to reword commit messages in this PR, but something to keep in mind for the future.

Member

23Skidoo commented Jul 5, 2017

And by the way, can you please format your commit messages in the standard way? See this article: https://chris.beams.io/posts/git-commit/ for an explanation of what that means. No need to reword commit messages in this PR, but something to keep in mind for the future.

@fgaz

This comment has been minimized.

Show comment
Hide comment
@fgaz

fgaz Jul 5, 2017

Collaborator

@23Skidoo
Sorry, I don't know why but I assumed the standard merging procedure here was to squash the commits, so I didn't pay much attention to the messages. Will do next time, thanks for the heads up.

Collaborator

fgaz commented Jul 5, 2017

@23Skidoo
Sorry, I don't know why but I assumed the standard merging procedure here was to squash the commits, so I didn't pay much attention to the messages. Will do next time, thanks for the heads up.

Add tests for new-run
We test both for single and multiple exe/package.

`new-run` must halt on ambiguities or no compatible exes,
and must run the exe if one can be uniquely selected between
packages and components.
@fgaz

This comment has been minimized.

Show comment
Hide comment
@fgaz

fgaz Jul 5, 2017

Collaborator

Tests ready!
If they fail it's probably because of #4583

Collaborator

fgaz commented Jul 5, 2017

Tests ready!
If they fail it's probably because of #4583

# cabal new-run
Up to date
# cabal new-run
cabal: The run command is for running a single executable at once. The target '' refers to the package MultipleExes-1.0 which includes the executables foo and bar.

This comment has been minimized.

@fgaz

fgaz Jul 5, 2017

Collaborator

And here's the need for #4583

@fgaz

fgaz Jul 5, 2017

Collaborator

And here's the need for #4583

-> Bool
matchPackage pkgId pkg =
pkgId == Just (elabPkgSourceId pkg)
|| isNothing pkgId --if the package is unspecified (Nothing), all packages match

This comment has been minimized.

@dmwit

dmwit Jul 5, 2017

Collaborator

Slight bikeshed: matchPackage pkgId pkg = maybe True (elabPkgSourceId pkg ==) pkgId seems to say what you want more concisely. Might be worth pulling out

maybeMatches :: Eq a => a -> Maybe a -> Bool
maybeMatches x = maybe True (x==)

since matchComponent has very similar code.

@dmwit

dmwit Jul 5, 2017

Collaborator

Slight bikeshed: matchPackage pkgId pkg = maybe True (elabPkgSourceId pkg ==) pkgId seems to say what you want more concisely. Might be worth pulling out

maybeMatches :: Eq a => a -> Maybe a -> Bool
maybeMatches x = maybe True (x==)

since matchComponent has very similar code.

This comment has been minimized.

@fgaz

fgaz Jul 6, 2017

Collaborator

Looks ok, but I'd prefer to flip the arguments (so the "thing to search/filter for" is first as usual).

@fgaz

fgaz Jul 6, 2017

Collaborator

Looks ok, but I'd prefer to flip the arguments (so the "thing to search/filter for" is first as usual).

This comment has been minimized.

@fgaz

fgaz Jul 6, 2017

Collaborator

I'm also tempted to change Maybe with something like
data Match a = Anything | Exactly a
but losing all the functions in Data.Maybe probably isn't worth it.

@fgaz

fgaz Jul 6, 2017

Collaborator

I'm also tempted to change Maybe with something like
data Match a = Anything | Exactly a
but losing all the functions in Data.Maybe probably isn't worth it.

@23Skidoo

Left some minor comments.

Show outdated Hide outdated Cabal/doc/nix-local-build.rst Outdated
Show outdated Hide outdated cabal-install/changelog Outdated
Show outdated Hide outdated cabal-install/Distribution/Client/CmdRun.hs Outdated
let args = drop 1 targetStrings
runProgramInvocation
verbosity
(simpleProgramInvocation exePath args)

This comment has been minimized.

@23Skidoo

23Skidoo Jul 14, 2017

Member

Do we have a test case that checks that new-run propagates the exit code?

@23Skidoo

23Skidoo Jul 14, 2017

Member

Do we have a test case that checks that new-run propagates the exit code?

@23Skidoo

This comment has been minimized.

Show comment
Hide comment
@23Skidoo

23Skidoo Jul 14, 2017

Member

Would be nice if @dcoutts could take a look too.

Member

23Skidoo commented Jul 14, 2017

Would be nice if @dcoutts could take a look too.

@fgaz

This comment has been minimized.

Show comment
Hide comment
@fgaz

fgaz Jul 14, 2017

Collaborator

Once I fix those minor problems there are two issues I'd like to address before merging:

The args issue.

I thought about it, and i think the old-run command solved the problem well. I often add a cabal flag after the exe name, and the -- syntax is standard. This conflicts with the <empty-target> -- args syntax which I don't think is that useful, so in my opinion we should keep it as it is.

Returning the target from runProjectPreBuildPhase
There is duplication between the logic in the function passed to runProjectPreBuildPhase and around line 170. We check for multiple/no targets two times (the second one assuming everything will go well) when we could simply return the single target from runProjectPreBuildPhase.

But in the only function we pass to it we can only alter the ElaboratedInstallPlan, so i think we should change its type to one of the following:

  • .... -> (ElabInstPlan -> (ElabInstPlan, arbitraryData) -> ... -> IO (ProjectBuildContext, arbitraryData)
    which is probably bad because that type variable only calls for misuse
  • .... -> (ElaboratedInstallPlan -> (ElaboratedInstallPlan, Targets) -> ... -> IO ProjectBuildContext
  • .... ((ElaboratedInstallPlan, ProjectBuildContext )-> (ElaboratedInstallPlan, ProjectBuildContext) -> ... -> IO ProjectBuildContext
    and put the target in a new field in ProjectBuildContext.
    This way future modifications are also easy.

I think @dcoutts has some more info about this.

This last issue is only an implementation problem and does not affect the behaviour of new-run, so if you are all ok with this I'll open another issue for this and finally merge this one (once I've fixed those typos and tests >.<) so we can have a working new-run and solve that problem when we know more.

Collaborator

fgaz commented Jul 14, 2017

Once I fix those minor problems there are two issues I'd like to address before merging:

The args issue.

I thought about it, and i think the old-run command solved the problem well. I often add a cabal flag after the exe name, and the -- syntax is standard. This conflicts with the <empty-target> -- args syntax which I don't think is that useful, so in my opinion we should keep it as it is.

Returning the target from runProjectPreBuildPhase
There is duplication between the logic in the function passed to runProjectPreBuildPhase and around line 170. We check for multiple/no targets two times (the second one assuming everything will go well) when we could simply return the single target from runProjectPreBuildPhase.

But in the only function we pass to it we can only alter the ElaboratedInstallPlan, so i think we should change its type to one of the following:

  • .... -> (ElabInstPlan -> (ElabInstPlan, arbitraryData) -> ... -> IO (ProjectBuildContext, arbitraryData)
    which is probably bad because that type variable only calls for misuse
  • .... -> (ElaboratedInstallPlan -> (ElaboratedInstallPlan, Targets) -> ... -> IO ProjectBuildContext
  • .... ((ElaboratedInstallPlan, ProjectBuildContext )-> (ElaboratedInstallPlan, ProjectBuildContext) -> ... -> IO ProjectBuildContext
    and put the target in a new field in ProjectBuildContext.
    This way future modifications are also easy.

I think @dcoutts has some more info about this.

This last issue is only an implementation problem and does not affect the behaviour of new-run, so if you are all ok with this I'll open another issue for this and finally merge this one (once I've fixed those typos and tests >.<) so we can have a working new-run and solve that problem when we know more.

@fgaz

This comment has been minimized.

Show comment
Hide comment
@fgaz

fgaz Jul 15, 2017

Collaborator

All done here, I'm merging this by tomorrow. I'll also open another pr for the duplication issue.

Collaborator

fgaz commented Jul 15, 2017

All done here, I'm merging this by tomorrow. I'll also open another pr for the duplication issue.

@fgaz fgaz merged commit 7327c12 into haskell:master Jul 16, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@fgaz fgaz referenced this pull request Jul 17, 2017

Merged

Reduce duplication in CmdRun #4607

@fgaz fgaz moved this from current to done in Last Mile for `cabal new-build` (HSOC2017) Jul 19, 2017

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