Rebuild source directories added to the sandbox. #1118

Merged
merged 8 commits into from Dec 10, 2012

Conversation

Projects
None yet
3 participants
Member

23Skidoo commented Nov 16, 2012

Adds an offline mode for the install command and an onlyDepsOf flag that allows to remove a subset of targets from the install plan (see also my message to the mailing list).

Rebuilding of add-source deps themselves is implemented by running

install --offline --only-dependencies-of="." "." /path/to/add-source-dep-1 ... /path/to/add-source-dep-N

before cabal build.

Owner

tibbe commented Nov 16, 2012

@dcoutts I'd really like your input on this one.

@23Skidoo To avoid UI discussion issues, couldn't this all be done internally, instead of calling some command with some new command line flags?

Here's how I imagine what needs to be done. Each of these could be implemented as a function or two:

  1. Create an install plan for the package (just like cabal install already does).
  2. Remove the package itself (just like --only-dependencies).
  3. Remove already installed packages (just like cabal install already does).
  4. Figure out which add-source dependencies need to be rebuilt. This is currently all dependencies as we don't track enough information to know what to rebuild.
  5. Find all reverse dependencies (except the main package) of the add-source dependencies and add them to the plan.
  6. Run cabal install --force-reinstalls on this set of packages.

I realize that this is probably what you're already doing (I haven't reviewed the code in depth yet). I think it would be useful to be able to do this internally in cabal, by calling functions such as reverseDepedenciesOf :: Package -> [Package] and addToInstallPlan :: Plan -> Package -> Plan or something along those lines.

Member

23Skidoo commented Nov 16, 2012

To avoid UI discussion issues, couldn't this all be done internally, instead of calling some command with some new command line flags?

This is basically how it is done. In sandboxBuild, I call Distribution.Client.Install.install and set installFlags to

let installFlags = mempty { installOfflineMode       = Flag True,
                            installOnlyDepsOf        = Flag ["."] }

Now, installOfflineMode is exposed as the --offline option for install, but this is not really necessary for my code to work (I just decided that it might be useful). installOnlyDepsOf is not exposed at all since it is so esoteric.

Owner

tibbe commented Nov 16, 2012

@23Skidoo What could happen if we didn't use the --offline flag?

Member

23Skidoo commented Nov 16, 2012

What could happen if we didn't use the --offline flag?

The --offline flag prohibits downloading packages from the Internet. Without --offline, the first run of cabal sandbox-build is equivalent to cabal sandbox-install --only-dependencies && cabal build.

Owner

tibbe commented Nov 16, 2012

The --offline flag prohibits downloading packages from the Internet. Without --offline, the first run of cabal sandbox-build is equivalent to cabal sandbox-install --only-dependencies && cabal build.

OK. So I guess we want this behavior until/if we decide to have build imply installing dependencies (from the internet).

Member

23Skidoo commented Nov 16, 2012

OK. So I guess we want this behavior until/if we decide to have build imply installing dependencies (from the internet).

Yes, then it won't be needed (though we might allow the users to enable offline mode explicitly).

Owner

tibbe commented Nov 16, 2012

Yes, then it won't be needed (though we might allow the users to enable offline mode explicitly).

Yes. Explicitly disabling package downloads using a flag is something that would be nice to support.

Member

23Skidoo commented Nov 16, 2012

Regarding this part of your comment:

  1. Figure out which add-source dependencies need to be rebuilt. This is currently all dependencies as we don't track enough information to know what to rebuild.
  2. Find all reverse dependencies of the add-source dependencies and add them to the plan.

This is done by the solver itself because the current package and all add-source dependencies are install targets. The only thing that remains is to remove the current package from the install plan (achieved with --only-dependencies-of).

Owner

tibbe commented Nov 19, 2012

Sorry for the radio silence on this issue. I'm still trying to wrap my head around it. There's something about the way we're implementing this that doesn't rub me the right way. Let me try to explain my current thinking:

I think (part of) the issue is that cabal install is context sensitive. cabal install --only-dependencies only make sense relative to some current working directory that you're currently in. This gives rise to weird cases. For example, what does

cabal install --only-dependencies foo

mean if the current package doesn't depend on foo? Then we're no longer installing "only" dependencies. In fact, I don't think the --only-dependencies flag has any effect once you start listing explicit package names (this is the weird mode switch from being about the current package to being about other packages cabal does).

I think what we're actually doing when we're rebuilding add-source packages is:

cabal install add-source1 add-source2
cabal install --reverse-dependencies add-source1 add-source2 --exclude=.

I don't have a good understanding if the above --reverse-dependencies flag is hard to implement or not. I think it would be useful even for people who don't use sandboxing (e.g. when they upgrade bytestring) and there's precedence for it in e.g. Gentoo (which has a revdep-rebuild command). There's also an issue of what happend if --exclude excludes a package that's needed by some other reverse dependency (that will then fail to build).

Member

dcoutts commented Nov 19, 2012

Isn't what we want to do, to make an install plan for the whole sandbox, so that we know we're always doing everything consistently. Then we prune that install plan graph back to the dependencies of the target(s).

I don't see why we need an offline mode for that (though I have considered it in the past as an independently useful feature, but I'm pretty sure it ought to be orthogonal here). I also don't think we need to add any command line flags for this. We ought to be able to do it just as manipulations of the install plan.

Member

dcoutts commented Nov 19, 2012

I think it might help if you called the individual parts/phases of install directly, rather than going via the high-level install action.

That is, call the planner directly, get back the InstallPlan, transform the InstallPlan and then use the existing machinery for executing the modified InstallPlan. So we should export more of the things from the Install module, and we'll have to add some queries to the InstallPlan module, so that it becomes possible to implement the transforms in terms of its api. (InstallPlan transforms should still go via the dynamic validity check.)

Member

23Skidoo commented Nov 19, 2012

@tibbe

In fact, I don't think the --only-dependencies flag has any effect once you start listing explicit package names

No, it has the same effect - create an install plan and then prune the install targets:

$ cabal install --only-dependencies --dry-run ghc-core
Resolving dependencies...
In order, the following would be installed (use -v for more details):
ansi-terminal-0.5.5
haskell-lexer-1.0
colorize-haskell-1.0.1
pcre-light-0.4

There's also an issue of what happend if --exclude excludes a package that's needed by some other reverse dependency (that will then fail to build).

This will make the install plan invalid and install will exit with an error.

I don't have a good understanding if the above --reverse-dependencies flag is hard to implement or not.

If someone can point me in the right direction, I can try my hand at it.

@dcoutts

I don't see why we need an offline mode for that

So that to prohibit build to access Internet. It's not strictly necessary.

Isn't what we want to do, to make an install plan for the whole sandbox, so that we know we're always doing everything consistently.

Can you advise me how to implement this without unnecessarily reinstalling everything? By default, we already refuse to proceed if the reinstall is likely to break something.

I also don't think we need to add any command line flags for this. We ought to be able to do it just as manipulations of the install plan.

Just to clarify: the only command-line flag I added is --offline. installOnlyDepsOf is a member of the InstallFlags record that is only accessible internally.

I think it might help if you called the individual parts/phases of install directly, rather than going via the high-level install action.

Maybe. Should I break up install into a set of smaller functions and remove installOnlyDepsOf?

Owner

tibbe commented Nov 19, 2012

Maybe. Should I break up install into a set of smaller functions and remove installOnlyDepsOf?

I think so. I think we want to create the functions necessary to be able to create the InstallPlan we need directly, without jumping through hoops such as --force-reinstalls etc. To do that we need some API on top of InstallPlan, such as addReverseDepedenciesOf and removePackage (these are just examples). The implementation of build for the sandbox would call some of these and create an InstallPlan that contains exactly what we want to build and then hand it off to some other part of cabal that builds it.

Member

23Skidoo commented Nov 19, 2012

I think so. I think we want to create the functions necessary to be able to create the InstallPlan we need directly, without jumping through hoops such as --force-reinstalls etc.

OK. It will take me some time.

Owner

tibbe commented Nov 19, 2012

OK. It will take me some time.

I appreciate that this is more difficult and perhaps not as easy as you hoped, but I do think it's the right thing to do and it will make cabal better overall.

Member

23Skidoo commented Nov 22, 2012

Should I still leave the --offline install option in?

Member

23Skidoo commented Nov 23, 2012

@dcoutts
I've removed the installOnlyDepsOf flag and made the InstallPlan manipulations more explicit (turned out to be easier than I expected). Please tell me if there's something more to be done.

@tibbe tibbe commented on the diff Nov 30, 2012

Cabal/Distribution/Simple/Utils.hs
@@ -316,6 +316,14 @@ debug verbosity msg =
putStr (wrapText msg)
hFlush stdout
+-- | A variant of 'debug' that doesn't perform the automatic line
+-- wrapping. Produces better output in some cases.
+debugNoWrap :: Verbosity -> String -> IO ()
+debugNoWrap verbosity msg =
+ when (verbosity >= deafening) $ do
+ putStrLn msg
@tibbe

tibbe Nov 30, 2012

Owner

debug doesn't add an extra newline. Do we really want to add one here?

@23Skidoo

23Skidoo Nov 30, 2012

Member

debug itself doesn't, but wrapText does.

@tibbe tibbe and 1 other commented on an outdated diff Nov 30, 2012

cabal-install/Distribution/Client/Install.hs
@@ -392,6 +335,22 @@ checkPrintPlan verbosity installed installPlan sourcePkgDb
else unless dryRun $ warn verbosity
"Note that reinstalls are always dangerous. Continuing anyway..."
+ -- If we are explicitly told to not download anything, check that all packages
+ -- are already fetched.
+ let offline = fromFlagOrDefault False (installOfflineMode installFlags)
+ when offline $ do
+ let pkgs = [ sourcePkg
+ | InstallPlan.Configured (ConfiguredPackage sourcePkg _ _ _)
+ <- InstallPlan.toList installPlan ]
+ notFetched <- fmap (map packageInfoId)
+ . filterM (fmap isNothing . checkFetched . packageSource)
+ $ pkgs
+ when (not . null $ notFetched) $
@tibbe

tibbe Nov 30, 2012

Owner

unless might be clearer than when . not.

@tibbe tibbe and 1 other commented on an outdated diff Nov 30, 2012

cabal-install/Distribution/Client/Sandbox.hs
-- Check that the sandbox exists.
- (sandboxDir, _) <- tryLoadSandboxConfig verbosity
-
+ (sandboxDir, pkgEnv) <- tryLoadSandboxConfig verbosity
+ indexFile <- tryGetIndexFilePath pkgEnv
+ buildTreeRefs <- Index.listBuildTreeRefs verbosity indexFile
+
+ -- Install all add-source dependencies of the current package into the
+ -- sandbox.
+ let installFlags = mempty { installOfflineMode = Flag True }
+ when (not . null $ buildTreeRefs) $
@tibbe

tibbe Nov 30, 2012

Owner

unless might be clearer than when . not.

@tibbe tibbe and 1 other commented on an outdated diff Nov 30, 2012

cabal-install/Main.hs
@@ -389,10 +400,47 @@ installAction (configFlags, configExFlags, installFlags, haddockFlags)
savedInstallFlags config `mappend` installFlags
globalFlags' = savedGlobalFlags config `mappend` globalFlags
(comp, conf) <- configCompilerAux' configFlags'
- install verbosity
- (configPackageDB' configFlags') (globalRepos globalFlags')
- comp conf globalFlags' configFlags' configExFlags' installFlags' haddockFlags
- targets
+
+ let -- For install, if no target is given it means we use the
@tibbe

tibbe Nov 30, 2012

Owner

Perhaps we could still keep the install function and use it here, but not in the sandbox building?

@23Skidoo

23Skidoo Nov 30, 2012

Member

Since we'll be merging sandbox-install and install anyway I decided not to keep it.

@tibbe

tibbe Dec 3, 2012

Owner

I would expect us to merge the code for sandboxing into the current code for e.g. configure, build, and install.

@23Skidoo

23Skidoo Dec 3, 2012

Member

Since @dcoutts also wants to keep the install function, I'll solve this differently.

Owner

tibbe commented Nov 30, 2012

Overall the change looks good to me. @dcoutts could you also take a look please? Also @kosmikus as well?

Owner

tibbe commented Dec 3, 2012

I'm inclined to merge this. @dcoutts any objections?

Member

dcoutts commented Dec 3, 2012

I don't see why we're moving the 'install' code into Main. It's fine to export the component parts from the Install module, but why move the main part? The general organisation is that the Main module really only deals with command line args, config files etc, and the logic of each command lives in the corresponding module. I think it's sensible to keep that.

Member

dcoutts commented Dec 3, 2012

I still don't think we need an offline mode for this. The way I think it should work is that we construct the install plan when we configure, then we keep that install plan as long as it remains valid (ie until someone changes one of the .cabal files in the source trees). Then build always just uses the same install plan, but we adjust which bits of it we actually want to build, given the current target(s).

That said, an offline mode is independently useful, though I think we would implement it differently. We'd tell the solver to avoid all packages that are not already fetched.

Ok, lets talk interim solutions since perhaps my suggestion to shift the planning into the configure phase and reusing the same plan (without re-running the solver) during build is too big a step to do immediately. Lets commit what we have now with the following changes:

  • no offline mode
  • keep the existing install code in the Install module (the exporting of more stuff is fine)
Member

23Skidoo commented Dec 3, 2012

@dcoutts I've added offline mode because @kosmikus was against having commands other than cabal install download packages from the Internet. This can happen e.g. if the user edits the .cabal file of an add-source package and adds a new dependency. The next cabal build will fetch and install that dependency into the sandbox. If this behaviour is considered acceptable, the offline mode can be dropped.

Member

dcoutts commented Dec 6, 2012

In a context where we're handling a whole set of local packages, I don't see a sensible alternative to making 'build' build dependencies and that may involve getting them from the network. Perhaps we could move the network part earlier to the configure phase, so it fetches them at that point. That may well be a good idea but that doesn't seem like a very significant difference, since if you do a 'build' and the configuration is out of date (because the user changed a .cabal file) then we're going to want to automatically reconfigure anyway.

If we default to an offline mode by default for that, then it just means we're likely to fail instead. As a longer term thing, I've been thinking of making some of the UI interactive by default, so that it gets confirmation for some things, like confirming when new packages will be installed.

Member

23Skidoo commented Dec 6, 2012

I'll remove the offline mode then.

Member

23Skidoo commented Dec 9, 2012

@dcoutts @tibbe I've updated my patches. Main changes:

  • Offline mode removed
  • Added an alternative interface for D.C.Install.install that allows to manipulate the install plan

23Skidoo added some commits Dec 9, 2012

@23Skidoo 23Skidoo Add an alternative interface for 'D.C.Install.install'.
Splits 'D.C.Install.install' into three parts:

    * makeInstallContext - load common data
    * makeInstallPlan    - produce the install plan
    * processInstallPlan - actually perform the installations

This allows to manipulate the install plan produced with 'makeInstallPlan'
before performing the installations with 'processInstallPlan'. The high-level
'install' action is still present; most clients should use it instead.
7772ce9
@23Skidoo 23Skidoo Rebuild source directories added to sandbox.
Implemented by creating an install plan for ["add-source-dep-1", ...,
"add-source-dep-N", "."], pruning "." from this plan and then doing all
remaining installs in the plan before building the current package. This way,
all reverse dependencies of add-source packages needed to install the current
package are also reinstalled.
5417ddd

@tibbe tibbe and 1 other commented on an outdated diff Dec 10, 2012

cabal-install/Distribution/Client/Install.hs
+
+ installContext <- makeInstallContext verbosity args userTargets0
+ installPlan <- foldProgress logMsg die return =<<
+ makeInstallPlan verbosity args installContext
+
+ processInstallPlan verbosity args installContext installPlan
+ where
+ args :: InstallArgs
+ args = (packageDBs, repos, comp, conf,
+ globalFlags, configFlags, configExFlags, installFlags,
+ haddockFlags)
+
+ logMsg message rest = debugNoWrap verbosity message >> rest
+
+-- | Common context for makeInstallPlan and processInstallPlan.
+type InstallContext = ( PackageIndex, SourcePackageDb
@tibbe

tibbe Dec 10, 2012

Owner

A TODO for the future: we probably want to make InstallContext and InstallArgs into proper data types with documented fields.

@23Skidoo

23Skidoo Dec 10, 2012

Member

Thought about this. Not that hard to change.

@tibbe tibbe and 1 other commented on an outdated diff Dec 10, 2012

cabal-install/Distribution/Client/Install.hs
+ makeInstallPlan verbosity args installContext
+
+ processInstallPlan verbosity args installContext installPlan
+ where
+ args :: InstallArgs
+ args = (packageDBs, repos, comp, conf,
+ globalFlags, configFlags, configExFlags, installFlags,
+ haddockFlags)
+
+ logMsg message rest = debugNoWrap verbosity message >> rest
+
+-- | Common context for makeInstallPlan and processInstallPlan.
+type InstallContext = ( PackageIndex, SourcePackageDb
+ , [UserTarget], [PackageSpecifier SourcePackage] )
+
+-- | Initial arguments given to 'install' or 'makeInstallContext'.
@tibbe

tibbe Dec 10, 2012

Owner

Another future TODO: we should think about if we can turn there types and functions into abstractions instead of just ways to share code. For example: is InstallArgs a useful concept on its own? What does it represent?

@23Skidoo

23Skidoo Dec 10, 2012

Member

The InstallArgs type synonym is used only for making argument lists a bit shorter. It wouldn't be very hard to remove it.

@tibbe tibbe commented on the diff Dec 10, 2012

cabal-install/Distribution/Client/Install.hs
let -- For install, if no target is given it means we use the
-- current directory as the single target
userTargets | null userTargets0 = [UserTargetLocalDir "."]
| otherwise = userTargets0
pkgSpecifiers <- resolveUserTargets verbosity
- (fromFlag $ globalWorldFile globalFlags)
- (packageIndex sourcePkgDb)
- userTargets
-
+ (fromFlag $ globalWorldFile globalFlags)
+ (packageIndex sourcePkgDb)
+ userTargets
+
+ return (installedPkgIndex, sourcePkgDb, userTargets, pkgSpecifiers)
+
+-- | Make an install plan given install context and install arguments.
+makeInstallPlan :: Verbosity -> InstallArgs -> InstallContext
+ -> IO (Progress String String InstallPlan)
+makeInstallPlan verbosity
+ (_, _, comp, _,
@tibbe

tibbe Dec 10, 2012

Owner

All these _ patterns suggest that the grouping of these arguments together might not be the best possible one.

I'm really keen on making progress on this issue now to meet our promised end-of-year release date, so I'm inclined to leave any non-essential work as future TODOs, including this.

Owner

tibbe commented Dec 10, 2012

Overall the change looks good to me.

Member

dcoutts commented Dec 10, 2012

Ok, looks fine to me too.

About those type aliases, they're fine when the thing is basically an internal api, but we could look at them again when we consider making it more of a public api. For now it's fine.

@tibbe tibbe merged commit 7305ef4 into haskell:master Dec 10, 2012

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