Implement package environment file parsing #993

Merged
merged 22 commits into from Aug 16, 2012

Conversation

Projects
None yet
2 participants
Member

23Skidoo commented Aug 8, 2012

This series of patches implements the second of the two infrastructure changes required to add sandboxes (aka package environments, hermetic builds) to Cabal. It adds a new config file type - a package environment file, as described in [1]. The bulk of the implementation is contained in the new module Distribution.Client.PackageEnvironment.

Example of a package environment file:

-- inherit: /home/$USERNAME/.cabal/config
-- constraints: foo ==1.0, foo +use_blah -use_blurb
-- preferences: bar ==2.0
remote-repo: hackage.haskell.org:http://hackage.haskell.org/packages/archive
remote-repo-cache: /home/$USERNAME/.cabal/packages
local-repo: /path/tol/.cabal-sandbox/packages
logs-dir: /path/to/.cabal-sandbox/logs
world-file: /path/to/.cabal-sandbox/world
user-install: False
package-db: /path/to/.cabal-sandbox/packages.conf.d

install-dirs 
  prefix: /path/to/.cabal-sandbox
  -- bindir: $prefix/bin
  -- libdir: $prefix/lib
  -- libsubdir: $pkgid/$compiler
  -- libexecdir: $prefix/libexec
  -- datadir: $prefix/share
  -- datasubdir: $pkgid
  -- docdir: $datadir/doc/$pkgid
  -- htmldir: $docdir/html
  -- haddockdir: $htmldir

-- Other configuration:
-- optimization: True
-- with-compiler: /path/to/my-special-ghc

The package environment file looks a lot like ~/.cabal/config and supports the same configuration options. The differences are:

  • An inherit option to allow inheriting from ~/.cabal/config or other config files.
  • Options constraint and preference are replaced with constraints and preferences that are parsed as comma-separated lists.
  • A single install-dirs section instead of the user and the global one.
  • The default install directory is ./.cabal-sandbox instead of $HOME/.cabal.

[1] http://hackage.haskell.org/trac/hackage/wiki/PackageEnvironments

Member

23Skidoo commented Aug 9, 2012

Here it may be easier to just review the whole diff instead of going through each patch one-by-one.

Owner

tibbe commented Aug 9, 2012

@23Skidoo I prefer to do it patch by patch, as it gives me smaller (and hopefully somewhat orthogonal) chunks to work with.

Member

23Skidoo commented Aug 9, 2012

@tibbe OK. It may be also useful to look at the Distribution.Client.Config module, since there are a lot of similarities between it and Distribution.Client.PackageEnvironment. Maybe we can even merge these two modules in the future.

tibbe commented on 37d6bda Aug 13, 2012

LGTM

Owner

tibbe commented Aug 13, 2012

Next time, could you please rebase your changes such that they are less "incremental" (i.e. don't create a file over three partial commits). It would make my reviewing job much easier. Using the combined diff helps me a bit, but it makes the change kinda overwhelming to look at (and it doesn't make the dependencies of the different changes clear).

Owner

tibbe commented Aug 13, 2012

I've taken a first pass over all the code. I will have to go over everything again in more detail.

Member

23Skidoo commented Aug 14, 2012

Next time, could you please rebase your changes such that they are less "incremental" (i.e. don't create a file over three partial commits).

I tend to follow the "commit early, commit often" maxim, which does result in a lot of smaller incremental commits. Which commits do you want to see merged?

Member

23Skidoo commented Aug 14, 2012

@tibbe I think I've now addressed all your comments except commit squashing.

Owner

tibbe commented Aug 14, 2012

I tend to follow the "commit early, commit often" maxim, which does result in a lot of smaller incremental commits. Which commits do you want to see merged?

None for now. I think this is a good maxim while working on code, but sometimes it's useful to polish our commits before sending them off to help the reviewer. In particular I would try to having commits with a lot of open questions and/or todos in them that get resolved in later commits.

Think of it as writing an article. While writing you might jump back and forth and edit different paragraphs, but you want to present the reader with coherent story.

@tibbe tibbe commented on the diff Aug 14, 2012

cabal-install/cabal-install.cabal
@@ -87,6 +87,7 @@ Executable cabal
Distribution.Client.InstallPlan
Distribution.Client.InstallSymlink
Distribution.Client.List
+ Distribution.Client.PackageEnvironment
Distribution.Client.PackageIndex
Distribution.Client.PackageUtils
@tibbe

tibbe Aug 14, 2012

Owner

Need to add Distribution.Client.ParseUtils.

@tibbe tibbe commented on the diff Aug 14, 2012

cabal-install/Distribution/Client/PackageEnvironment.hs
+
+-- | Initial configuration that we write out to the package environment file if
+-- it does not exist. When the package environment gets loaded it gets layered
+-- on top of 'basePackageEnvironment'.
+initialPackageEnvironment :: FilePath -> IO PackageEnvironment
+initialPackageEnvironment pkgEnvDir = do
+ initialConf' <- initialSavedConfig
+ let baseConf = commonPackageEnvironmentConfig pkgEnvDir
+ let initialConf = initialConf' `mappend` baseConf
+ return $ mempty {
+ pkgEnvSavedConfig = initialConf {
+ savedGlobalFlags = (savedGlobalFlags initialConf) {
+ globalLocalRepos = [pkgEnvDir </> "packages"]
+ },
+ savedConfigureFlags = (savedConfigureFlags initialConf) {
+ -- TODO: This should include comp. flavor and version
@tibbe

tibbe Aug 14, 2012

Owner

I guess you mean that the file name needs to include these two things? To mirror how packages are installed today under a compiler flavor/version path?

@23Skidoo

23Skidoo Aug 14, 2012

Member

Yes, something like this.

@23Skidoo

23Skidoo Aug 14, 2012

Member

E.g., cabal-dev uses packages-7.4.2-conf. We probably want it to be something like .cabal-sandbox/package.conf.d/ghc/7.4.2.

@tibbe tibbe and 1 other commented on an outdated diff Aug 14, 2012

cabal-install/Distribution/Client/PackageEnvironment.hs
+ minp <- readPackageEnvironmentFile mempty path
+ case minp of
+ Nothing -> do
+ notice verbosity $ "Writing default package environment to " ++ path
+ commentPkgEnv <- commentPackageEnvironment pkgEnvDir
+ initialPkgEnv <- initialPackageEnvironment pkgEnvDir
+ writePackageEnvironmentFile path commentPkgEnv initialPkgEnv
+ return initialPkgEnv
+ Just (ParseOk warns pkgEnv) -> do
+ when (not $ null warns) $ warn verbosity $
+ unlines (map (showPWarning path) warns)
+ return pkgEnv
+ Just (ParseFailed err) -> do
+ let (line, msg) = locatedErrorMsg err
+ warn verbosity $
+ "Error parsing package environment file '" ++ path
@tibbe

tibbe Aug 14, 2012

Owner

Missing closing single quote.

@tibbe tibbe commented on the diff Aug 14, 2012

cabal-install/Distribution/Client/PackageEnvironment.hs
+ (\v pkgEnv -> updateConfigureExFlags pkgEnv
+ (\flags -> flags { configExConstraints = v }))
+
+ , commaListField "preferences"
+ Text.disp Text.parse
+ (configPreferences . savedConfigureExFlags . pkgEnvSavedConfig)
+ (\v pkgEnv -> updateConfigureExFlags pkgEnv
+ (\flags -> flags { configPreferences = v }))
+ ]
+ ++ map toPkgEnv configFieldDescriptions'
+ where
+ optional = Parse.option mempty . fmap toFlag
+
+ configFieldDescriptions' :: [FieldDescr SavedConfig]
+ configFieldDescriptions' = filter
+ (\(FieldDescr name _ _) -> name /= "preference" && name /= "constraint")
@tibbe

tibbe Aug 14, 2012

Owner

Just double checking: you intended "preference" and "constraint" in singular here and not plural, like above?

@23Skidoo

23Skidoo Aug 14, 2012

Member

Yes.

$ cabal help configure | grep -A1 constraint
    --constraint=CONSTRAINT        Specify constraints on a package (version,
                                   installed/source, flags)
    --preference=CONSTRAINT        Specify preferences (soft constraints) on
                                   the version of a package

@tibbe tibbe commented on the diff Aug 14, 2012

cabal-install/Distribution/Client/PackageEnvironment.hs
+parsePackageEnvironment initial str = do
+ fields <- readFields str
+ let (knownSections, others) = partition isKnownSection fields
+ pkgEnv <- parse others
+ let config = pkgEnvSavedConfig pkgEnv
+ installDirs0 = savedUserInstallDirs config
+ installDirs <- foldM parseSection installDirs0 knownSections
+ return pkgEnv {
+ pkgEnvSavedConfig = config {
+ savedUserInstallDirs = installDirs,
+ savedGlobalInstallDirs = installDirs
+ }
+ }
+
+ where
+ isKnownSection :: ParseUtils.Field -> Bool
@tibbe

tibbe Aug 14, 2012

Owner

Could you please add a comment somewhere describing why install-dirs is special-cased? Is it simply the only know section or is it special in some other way?

@23Skidoo

23Skidoo Aug 14, 2012

Member

The former.

Owner

tibbe commented Aug 14, 2012

Almost there! It would be great if @dcoutts and/or @kosmikus could take one quick look before we commit this (after you addressed my last few comments).

@dcoutts and/or @kosmikus, could you take a quick look at the overall logic of the patch. You can get a good overview of everything that's going on here by using the Diff tab above.

Member

23Skidoo commented Aug 14, 2012

Think of it as writing an article. While writing you might jump back and forth and edit different paragraphs, but you want to present the reader with coherent story.

Thanks for the advice, I'll try to keep this in mind.

Member

23Skidoo commented Aug 14, 2012

@tibbe I believe that I've now addressed all your comments.

Owner

tibbe commented Aug 15, 2012

I'm happy with these commits now and I'm happy to merge them. I would like to hear from @kosmikus or @dcoutts though.

@tibbe tibbe merged commit 6f56684 into haskell:master Aug 16, 2012

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