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

new-run #4586

Merged
merged 26 commits into from
Jul 16, 2017
Merged

new-run #4586

Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
235 changes: 232 additions & 3 deletions cabal-install/Distribution/Client/CmdRun.hs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ module Distribution.Client.CmdRun (
selectComponentTarget
) where

import Prelude ()
import Distribution.Client.Compat.Prelude

import Distribution.Client.ProjectOrchestration
import Distribution.Client.CmdErrorMessages

Expand All @@ -30,11 +33,39 @@ import Distribution.Text
import Distribution.Verbosity
( Verbosity, normal )
import Distribution.Simple.Utils
( wrapText, die', ordNub )
( wrapText, die', ordNub, info )
import Distribution.Types.PackageName
( unPackageName )
import Distribution.Client.ProjectPlanning
( ElaboratedConfiguredPackage(..)
, ElaboratedInstallPlan )
import Distribution.Client.InstallPlan
( toList, foldPlanPackage )
import Distribution.Client.ProjectPlanning.Types
( ElaboratedPackageOrComponent(..)
, ElaboratedComponent(compComponentName)
, BuildStyle(BuildInplaceOnly, BuildAndInstall)
, ElaboratedSharedConfig, elabDistDirParams )
import Distribution.Types.Executable
( Executable(exeName) )
import Distribution.Types.UnqualComponentName
( UnqualComponentName, unUnqualComponentName )
import Distribution.Types.PackageDescription
( PackageDescription(executables) )
import Distribution.Simple.Program.Run
( runProgramInvocation, simpleProgramInvocation )
import Distribution.Types.PackageId
( PackageIdentifier(..) )
import Distribution.Client.DistDirLayout
( DistDirLayout, distBuildDirectory )
import qualified Distribution.Simple.InstallDirs as InstallDirs

import qualified Data.Map as Map
import qualified Data.Set as Set
import Control.Monad (when)
import Data.Function
( on )
import System.FilePath
( (</>) )


runCommand :: CommandUI (ConfigFlags, ConfigExFlags, InstallFlags, HaddockFlags)
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

(take 1 targetStrings) -- we drop the exe's args
Copy link
Contributor

Choose a reason for hiding this comment

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

Urk. Wouldn't it be better to split targetStrings first before doing anything with it? See (A).

Copy link
Member Author

Choose a reason for hiding this comment

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

See (A).


buildCtx <-
runProjectPreBuildPhase verbosity baseCtx $ \elaboratedPlan -> do
Expand Down Expand Up @@ -128,12 +160,209 @@ runAction (configFlags, configExFlags, installFlags, haddockFlags)

buildOutcomes <- runProjectBuildPhase verbosity baseCtx buildCtx
runProjectPostBuildPhase verbosity baseCtx buildCtx buildOutcomes

-- We get the selectors for the package and component.
-- These are wrapped in Maybes, because the user
-- might not specify them
(selectedPackage, selectedComponent) <-
-- this should always match [x] anyway because
-- we already check for a single target in TargetSelector.hs
case selectorPackageAndComponent <$> targetSelectors
of [x] -> return x
[ ] -> die'
verbosity
"No targets given"
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this error message occur when I type cabal new-run? I think you can give a more informative message here.

Copy link
Member Author

Choose a reason for hiding this comment

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

(B2) No arguments means "treat the package in the cwd/project as a target, if there is one" (look at the comment above extractMatchingElaboratedConfiguredPackages). But this is also handled before, so same as (B1).
This will work after #4583 is merged.
I'll add a test for it too.

_ -> die'
verbosity
"Multiple targets given"
Copy link
Contributor

Choose a reason for hiding this comment

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

This cannot happen, by virtue of the take 1, correct? In that case, better to mark it as an internal error.

Copy link
Member Author

Choose a reason for hiding this comment

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

(B1) Not by virtue of the take 1. For example if we specify a package with 2 exes in the targetStrings there will be two targets. But this is handled by lines 138-149 already, so you are correct. Should iI use an assert?

Copy link
Member Author

@fgaz fgaz Jul 5, 2017

Choose a reason for hiding this comment

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

Wait, did i just duplicate resolveTargets' functionality? >.<
edit: Nvm, it doesn't return enough information. I think I already checked it some time ago.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think "multiple targets" is a bit misleading. It's more like a single, ambiguous target. Perhaps the error could be improved by mentioning longer target strings the user might want to try to disambiguate between the possibilities, e.g. "Ambiguous target my-package; could match my-package:my-exe1 or my-package:my-exe2" or "Ambiguous target my-exe; could match my-package1:my-exe or my-package2:my-exe". (Or am I misunderstanding when we would hit this branch?)

Copy link
Member Author

Choose a reason for hiding this comment

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

@dmwit: as @ezyang says we shouldn't hit that branch. It's allalready handled with better error reporting before.


let elaboratedPlan = elaboratedPlanOriginal buildCtx
matchingElaboratedConfiguredPackages =
extractMatchingElaboratedConfiguredPackages
selectedPackage
selectedComponent
elaboratedPlan

-- the names to match. used only for user feedback, as
Copy link
Member

Choose a reason for hiding this comment

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

Please use proper punctuation in comments (i.e. start sentences with capital letters, end them with full stops, and so on).

-- later on we extract the real ones (whereas these are
-- wrapped in a Maybe) from the package itself
let selectedPackageNameToMatch = getPackageName <$> selectedPackage
selectedComponentNameToMatch = getExeComponentName =<< selectedComponent

-- For each ElaboratedConfiguredPackage in the install plan, we
-- identify candidate executables. We only keep them if both the
-- package name and executable name match what the user asked for
-- (a missing specification matches everything).
--
-- In the common case, we expect this to pick out a single
-- ElaboratedConfiguredPackage that provides a single way of building
-- an appropriately-named executable. In that case we prune our
-- install plan to that UnitId and PackageTarget and continue.
--
-- However, multiple packages/components could provide that
-- executable, or it's possible we don't find the executable anywhere
-- in the build plan. I suppose in principle it's also possible that
-- a single package provides an executable in two different ways,
-- though that's probably a bug if. Anyway it's a good lint to report
-- an error in all of these cases, even if some seem like they
-- shouldn't happen.
(pkg,exe) <- case matchingElaboratedConfiguredPackages of
[] -> die' verbosity $ "Unknown executable"
++ case selectedComponentNameToMatch
of Just x -> " " ++ x
Nothing -> ""
++ case selectedPackageNameToMatch
of Just x -> " in package " ++ x
Nothing -> ""
[(elabPkg,exe)] -> do
info verbosity $ "Selecting " ++ display (elabUnitId elabPkg)
++ case selectedComponentNameToMatch
of Just x -> " to supply " ++ x
Nothing -> ""
return (elabPkg, unUnqualComponentName exe)
elabPkgs -> die' verbosity
$ "Multiple matching executables found"
++ case selectedComponentNameToMatch
of Just x -> " matching " ++ x
Nothing -> ""
++ ":\n"
++ unlines (fmap (\(p,_) -> " - in package " ++ display (elabUnitId p)) elabPkgs)
let exePath = binDirectoryFor (distDirLayout baseCtx)
(elaboratedShared buildCtx)
pkg
exe
</> exe
let args = drop 1 targetStrings
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@dmwit dmwit Jul 5, 2017

Choose a reason for hiding this comment

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

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.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea.

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@fgaz fgaz Jul 10, 2017

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

runProgramInvocation
verbosity
(simpleProgramInvocation exePath args)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@fgaz fgaz Jul 13, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Now we have one

where
verbosity = fromFlagOrDefault normal (configVerbosity configFlags)
cliConfig = commandLineFlagsToProjectConfig
globalFlags configFlags configExFlags
installFlags haddockFlags

-- Package selection
------

getPackageName :: PackageIdentifier -> String
getPackageName (PackageIdentifier packageName _) =
unPackageName packageName

getExeComponentName :: ComponentName -> Maybe String
getExeComponentName (CExeName unqualComponentName) =
Just $ unUnqualComponentName unqualComponentName
getExeComponentName _ = Nothing

selectorPackageAndComponent :: TargetSelector PackageId
-> (Maybe PackageId, Maybe ComponentName)
selectorPackageAndComponent (TargetPackage _ pkg _) =
(Just pkg, Nothing)
selectorPackageAndComponent (TargetAllPackages _) =
(Nothing, Nothing)
selectorPackageAndComponent (TargetComponent pkg component _) =
(Just pkg, Just component)

-- | Extract all 'ElaboratedConfiguredPackage's and executable names
-- that match the user-provided component/package
-- The component can be either:
-- * specified by the user (both Just)
-- * deduced from an user-specified package (the component is unspecified, Nothing)
-- * deduced from the cwd (both the package and the component are unspecified)
extractMatchingElaboratedConfiguredPackages
:: Maybe PackageId -- ^ the package to match
-> Maybe ComponentName -- ^ the component to match
-> ElaboratedInstallPlan -- ^ a plan in with to search for matching exes
-> [(ElaboratedConfiguredPackage, UnqualComponentName)] -- ^ the matching package and the exe name
extractMatchingElaboratedConfiguredPackages
pkgId component = nubBy equalPackageIdAndExe
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

. catMaybes
. fmap sequenceA' -- get the Maybe outside the tuple
. fmap (\p -> (p, matchingExecutable p))
. catMaybes
. fmap (foldPlanPackage
(const Nothing)
(\x -> if match x
then Just x
else Nothing))
. toList
where
-- We need to support ghc 7.6, so we don't have
-- a sequenceA that works on tuples yet.
-- Once we drop support for pre-ftp ghc
-- it's safe to remove this.
sequenceA' (a, Just b) = Just (a, b)
sequenceA' _ = Nothing
match :: ElaboratedConfiguredPackage -> Bool
match p = matchPackage pkgId p && matchComponent component p
matchingExecutable p = atMostOne
$ filter (\x -> Just x == componentString
|| isNothing componentString)
$ executablesOfPackage p
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to change this, but my preference here would have been to case on p here for the package versus component case, rather than put everything through the "get the list of components it defines, and now check if anything in that list matches". My primary reasoning here is that the component case is more reflective of what the "good" code looks like, but the way things are wired up now it makes it seem like the package case (where multiple components may exist) is more important.

componentString = componentNameString =<< component
atMostOne [x] = Just x
atMostOne _ = Nothing
Copy link
Collaborator

Choose a reason for hiding this comment

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

exactlyOne?

Copy link
Member Author

Choose a reason for hiding this comment

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

or safeHead, or headMaybe...

yeah, that probably wasn't the best name :-P

equalPackageIdAndExe (p,c) (p',c') = c==c' && ((==) `on` elabPkgSourceId) p p'

matchPackage :: Maybe PackageId
-> ElaboratedConfiguredPackage
-> Bool
matchPackage pkgId pkg =
pkgId == Just (elabPkgSourceId pkg)
|| isNothing pkgId --if the package is unspecified (Nothing), all packages match
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


matchComponent :: Maybe ComponentName
-> ElaboratedConfiguredPackage
-> Bool
matchComponent component pkg =
componentString `elem` (Just <$> executablesOfPackage pkg)
|| isNothing componentString --if the component is unspecified (Nothing), all components match
where componentString = componentNameString =<< component

executablesOfPackage :: ElaboratedConfiguredPackage
-> [UnqualComponentName]
executablesOfPackage p =
case exeFromComponent
of Just exe -> [exe]
Nothing -> exesFromPackage
where
exeFromComponent =
case elabPkgOrComp p
of ElabComponent comp -> case compComponentName comp
of Just (CExeName exe) -> Just exe
_ -> Nothing
_ -> Nothing
exesFromPackage = fmap exeName $ executables $ elabPkgDescription p

-- Path construction
------

-- | The path to the @build@ directory for an inplace build.
inplaceBinRoot
:: DistDirLayout
-> ElaboratedSharedConfig
-> ElaboratedConfiguredPackage
-> FilePath
inplaceBinRoot layout config package
= distBuildDirectory layout (elabDistDirParams config package)
</> "build"

-- | The path to the directory that contains a specific executable.
binDirectoryFor
:: DistDirLayout
-> ElaboratedSharedConfig
-> ElaboratedConfiguredPackage
-> FilePath
-> FilePath
binDirectoryFor layout config package exe = case elabBuildStyle package of
BuildAndInstall -> installedBinDirectory package
BuildInplaceOnly -> inplaceBinRoot layout config package </> exe
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is duplicated with inplace_bin_dir in Distribution/Client/ProjectPlanning.hs. It would be good to have this in only one place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will merge.


-- package has been built and installed.
installedBinDirectory :: ElaboratedConfiguredPackage -> FilePath
installedBinDirectory = InstallDirs.bindir . elabInstallDirs


-- | This defines what a 'TargetSelector' means for the @run@ command.
-- It selects the 'AvailableTarget's that the 'TargetSelector' refers to,
-- or otherwise classifies the problem.
Expand Down
7 changes: 7 additions & 0 deletions cabal-testsuite/PackageTests/NewBuild/T4477/cabal.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# cabal new-run
Resolving dependencies...
In order, the following will be built:
- 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..
Copy link
Contributor

Choose a reason for hiding this comment

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

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!"

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

We also have -v0

3 changes: 1 addition & 2 deletions cabal-testsuite/PackageTests/NewBuild/T4477/cabal.test.hs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import Test.Cabal.Prelude
main = cabalTest $ do
expectBroken 4477 $ do
cabal' "new-run" ["foo"] >>= assertOutputContains "Hello World"
cabal' "new-run" ["foo"] >>= assertOutputContains "Hello World"