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

put executeInstallPlan into newly created Shell monad (#975) #1940

Closed
wants to merge 4 commits into from

Conversation

ghorn
Copy link

@ghorn ghorn commented Jun 13, 2014

This commit puts executeInstallPlan into a newly created Shell Monad, which tracks the status of the parallel install. It should be trivial to implement #975 by tweaking Distribution.Client.Shell. I realize this is quite a big change which also adds a dependency on exceptions, so I'd appreciate a review before this is merged.

commit messages follows:

The Shell monad is basically an IO monad which carries around the state
of the parallel install. It is an instance of MonadIO but you print output
through with special commands warn', notice', info', etc which will in the
future be redirected to write a nice UI.

Shell can also be forked with forkIO'.

This adds dependencies on transformers (which was already implied by the mtl
dependency) and exceptions (which is new but has no additional dependencies).

@23Skidoo
Copy link
Member

Cool stuff! I'll take a look.

/cc @dcoutts

This adds dependencies on transformers (which was already implied by the mtl
dependency) and exceptions (which is new but has no additional dependencies).

The latter might be problematic. In general, we try to restrict cabal-install's dependencies to what is in Platform.

@ghorn
Copy link
Author

ghorn commented Jun 13, 2014

The latter might be problematic. In general, we try to restrict cabal-install's dependencies to what is in Platform.

It should be relatively straightforward to manually lift the IO masking/bracketing functions into Shell and get rid of the typeclass. There is some overloading to deal with though, maybe we should just port those bits of exceptions to cabal-install.

void (forkIO' parsePipe)
return (writeIntercept, blockUntilFinished)

-- parse a line of the GHC build output, for example:
Copy link
Member

Choose a reason for hiding this comment

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

In theory we support other compilers than GHC, so you should check the compiler type and do something sensible if it's not GHC (e.g. fall back to old output mode).

Copy link
Author

Choose a reason for hiding this comment

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

I totally agree. Right now other compilers aren't necessarily broken, they will just have the module-level build status at PISBuilding Nothing instead of PISBuilding (Just (x,x,x))

Copy link
Member

Choose a reason for hiding this comment

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

So it'd be nice to somehow indicate that this code still works if the underlying compiler is different.

@23Skidoo
Copy link
Member

Another option is to move the Shell monad to the Cabal library and just use it everywhere instead of IO.

Then you could have

data ShellState = ShellState { 
                  ...
                  noticeHook :: Verbosity -> String -> IO (),
                  debugHook :: Verbosity -> String -> IO (),
                  .... }

type Shell a = ReaderT ShellState IO a

notice :: Verbosity -> String -> Shell ()
notice verbosity msg = do st <- ask
                          liftIO (noticeHook st verbosity msg)

This would also let us do things like keep the current working directory in the ShellState, and move back to using the internal setup method instead of setup exe cache for parallel installs.

scriptOptions { useLoggingHandle = logFileHandle
(\path -> Just `fmap` liftIO (openFile path AppendMode)) mLogPath)
(maybe (return ()) (liftIO . hClose))
(\logFileHandle0 -> do
Copy link
Member

Choose a reason for hiding this comment

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

To make this easier to review, it would be nice if you could split this into a separate patch.

So in the first patch, you'd have the Shell monad code and boring liftIO changes, and in the second you'd actually replace the old output mode with calls to updatePackageProgress & friends/add code for parsing GHC output etc.

Copy link
Author

Choose a reason for hiding this comment

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

No problem, should I delete the pull request and my master, and try again?

Copy link
Author

Choose a reason for hiding this comment

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

I actually have not replaced the old output mode, only wrapped it. I'll try to make that more clear

Copy link
Member

Choose a reason for hiding this comment

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

You can just do push --force, which will update this PR.

ghorn added 4 commits June 13, 2014 20:13
The Shell monad is basically an IO monad where you print output to the terminal
through with special commands warn', notice', info', etc. It is an instance of
MonadIO but you must not use this to print to the terminal.

Shell can be forked with forkIO'.

This adds dependencies on transformers (which was already implied by the mtl
dependency) and exceptions (which is new but has no additional dependencies).

This commit does the boring liftIO stuff, and puts Verbosity in Shell
as a ReaderT-like value. The commands warn', notice', info', etc are just
wrappers around the old values.
This only uses existing cabal machinery to track package-level progress.
Nothing here tracks module-level progress.
Because we call ghc --make, I am making a sort of tee and parsing
the output for package-level progress. Parse failures are handled
as described in the comments on `parseBuildLine`
@ghorn
Copy link
Author

ghorn commented Jun 13, 2014

ok here you go

@tibbe
Copy link
Member

tibbe commented Jun 15, 2014

@dcoutts have wanted to add a shell monad for a long time, so lets see what he thinks.

@dcoutts
Copy link
Contributor

dcoutts commented Jun 21, 2014

I'd love to have a shell monad in the Cabal lib itself, though initially in cabal-install is ok. Extra dependencies outside the platform are a problem. The lifting of exception handling into the monad may simply need to be copied in (I was agitating recently to have this added into the standard libs for exactly this reason).

As others have said, I want the shell monad for handling the output/logging stuff (& verbosity), invoking processes, virtualising cwd, env vars etc. So that stuff is fairly general and not much to do with parallel installs. In that kind of design a parallel install shell monad may want to be layered on top of the simple shell monad. Anyway, that's all a slightly grander design, which we don't have to do immediately but it's useful to know the direction of travel.

@23Skidoo
Copy link
Member

This seems to be stalled/inactive. Closing, but people are welcome to revive/continue this work.

@23Skidoo 23Skidoo closed this Sep 22, 2015
@ghorn
Copy link
Author

ghorn commented Sep 23, 2015

fair enough

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.

4 participants