Have the standard commands use the sandbox, if present. #1256

Closed
wants to merge 16 commits into
from

Projects

None yet

2 participants

Member

Fixes #1104.

I decided not to add support for subcommands to D.S.Command for the time being (cost/benefit considerations).

23Skidoo added some commits Mar 2, 2013
@23Skidoo 23Skidoo Remove the 'sandbox-*' commands.
Preparation for merging sandbox commands with their normal counterparts.
9fedc39
@23Skidoo 23Skidoo Comment. f5a6c72
@23Skidoo 23Skidoo New function: classifyPackageEnvironment. 0e39469
@23Skidoo 23Skidoo New function: loadUserConfig. 0c9284f
@23Skidoo 23Skidoo Add a ForceGlobalInstall argument to configPackageDB'.
We use userInstallDirs in sandbox mode to prevent cabal-install from doing
unnecessary things like invoking itself via 'sudo' (see commit
7b2e363 and 'rootCmd' in D.C.Install), but in
this particular case we want userInstall to be False to prevent UserPackageDB
from being added to the package DB stack (see #1183 and interpretPackageDbFlags
in D.S.Configure).
8426c6e
@23Skidoo 23Skidoo Add helpers for working with the PackageEnvironment type.
These allow us to work uniformly with @PackageEnvironment@ and @SavedConfig@,
thus reducing boilerplate.
3948dca
@23Skidoo 23Skidoo Merge the 'configure' and 'sandbox-configure' commands. 8a00ec1
@23Skidoo 23Skidoo Comment. a28da81
@23Skidoo 23Skidoo Merge the 'sandbox-install' and 'install' commands. 8cccfb3
@23Skidoo 23Skidoo Reindent for consistency with the rest of the file. d265ef4
@23Skidoo 23Skidoo Merge the 'sandbox-build' and 'build' commands. 38eea84
@23Skidoo 23Skidoo Add support for the sandbox mode to the remaining commands.
The commands that are implemented with 'wrapperAction' (haddock, hscolour,
register, clean, copy) don't require 'withSandboxDirOnPath', so their code is
not modified.
728f371
@23Skidoo 23Skidoo Reindent for consistency. 73e7be8
tibbe commented on 9fedc39 Mar 25, 2013

LGTM

tibbe commented on f5a6c72 Mar 25, 2013

LGTM

Does this imply that there's both a cabal.sandbox.config and a cabal.config present? We'd like users to be able to use both, since the former shouldn't be edited directly. A use case would be to set some constraints in cabal.config to make sure some specific versions of a package is used.

Owner

Yes, this means that there is a cabal.sandbox.config and optionally a cabal.config. See tryLoadPackageEnvironment in D.C.PackageEnvironment.

tibbe commented on 0c9284f Mar 25, 2013

LGTM

tibbe commented on 8426c6e Mar 25, 2013

LGTM

In the future we might instead want to have 3 kinds of install ways: global, user, and sandbox and have some predicate functions like useSudo :: InstallKind -> Bool.

This type confuses me. First, it looks like an Either rather than a Maybe, but the name suggests otherwise. Second, are SavedConfigs and PackageEnvironments interchangeable? If so, cannot we just convert between the two where we need to instead of passing around a sum type?

Owner

I added a NoPkgEnv constructor in a later commit (for the cases when the caller doesn't need to call loadConfig), so it's now more like Maybe. This type is used to indicate that there is a sandbox present, in which case the caller needs to do things a little differently.

It feels like this logic belongs in the caller instead of having a function that does two seemingly unrelated things.

Can't you just return a SavedConfig here instead of converting it later in this function (related to comment on prior commit)?

tibbe commented on a28da81 Mar 25, 2013

LGTM

As you see here you always seem to call toSavedConfig on the result of loadConfigOrPkgEnv, suggesting that that function should return a SavedConfig directly. It might also need to return some Bools to use with e.g. maybeInitPackageDBIfNeeded and maybeSetPackageDB.

Owner

Right, this type serves only to indicate that there is a sandbox present, which then enables these maybe* code paths.

tibbe commented on d265ef4 Mar 25, 2013

LGTM

Owner
tibbe commented Mar 25, 2013

The only bigger concern I have with the implementation is all this maybe/Maybe business. It feels like the caller (e.g. build) should be more explicit in that we return a useSandbox :: Bool and have things like:

when useSandbox $
    createBlaBla

instead of returning an opaque value from the Sandbox module that later gets passed back to that module.

@23Skidoo 23Skidoo Refactoring.
Replace the confusing MaybePkgEnv type with a (UseSandbox, SavedConfig)
tuple. Code in Main.hs is a bit uglier now.
446279a
Member

@tibbe I refactored the code based on your suggestions. Should be less confusing now.

Owner
tibbe commented Mar 25, 2013

It's clearer to me now at least. I think @dcoutts might have something to say, otherwise I'm happy with the changes (which are really important for the Haskell community!)

Member

Nice. I'll wait for @dcoutts's input then.

23Skidoo added some commits Mar 26, 2013
@23Skidoo 23Skidoo Rename 'usingSandbox' to 'isUseSandbox'.
Rationale: consistency with existing precedent ('isJust'/'isNothing').
bdd00e6
@23Skidoo 23Skidoo Make 'test' and 'bench' install add-source deps if we're in a sandbox.
Makes sense because they're basically doing 'cabal build' on each invocation.
38e72af
Owner
tibbe commented Apr 3, 2013

@dcoutts I would like to get a cabal release out the door with this feature included. What do you think?

Member
23Skidoo commented Apr 3, 2013

@tibbe

I would like to get a cabal release out the door with this feature included.

I also want the next release to include sandbox add-source --snapshot (#1143). I'm a bit busy right now, but I think that that feature will be ready in a week or two.

Owner
tibbe commented Apr 9, 2013

@dcoutts Did you want to take a look? Otherwise I will get this merged this week.

Owner
tibbe commented Apr 12, 2013

Merged! Merge commit: 9b77f56

@tibbe tibbe closed this Apr 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment