Consider reverting default usage of --enable-documentation in cabal haddock

[arybczak]

I was bitten by #8707 and I had no idea what was going on, had to search the issue tracker to find it.

Looks like now cabal haddock will with default settings (i.e. no documentation: True in the config file or --disable-documentation passed explicitly) lead to rebuild of all dependencies.

What is worse, if someone doesn't want to enable documentation: True, there seems to be no way to permanently disable this behavior.

This looks like a quite bad UX change to me.

Can this please be reverted? Or resolved in a better way.

[ulysses4ever]

As the author of the change in question, I'm happy with reverting, if people think that it's really annoying (and by the looks of it, they do).

[Mikolaj]

We've also had a lot of ticket from people bitten by the inverse default. Especially newcomers, and they need any help they can get. How can we resolve this in a better way? @arybczak: what would mollify you, for example?

[arybczak]

Well, for example having a way to disable this with a seting in ~/.cabal/config would work.

The issue here is that the new behavior can't be disabled without passing --disable-documentation to cabal haddock every time you invoke it.

[mouse07410]

I'm not sure it would be good to revert - for example, I always build documentation (and have documentation = True in the global config).

[arybczak]

for example, I always build documentation (and have documentation = True in the global config).

Great, then you're not affected by this change at all. Why would you be against the revert then?

EDIT: Another option would be changing documentation to True as a default. I just don't understand the rationale of having this unconditionally enabled in this specific case, especially since the global configuration file, judging by the amount of options it has, lets you configure everything under the sun.

[Bodigrim]

I find it extremely annoying to explain people, especially less experienced ones, that just cabal haddock is not enough and they must double down on their intention to build documentation with cabal haddock --enable-documentation. The sensible default behaviour is not to make users to ask you twice. Power users can always supply --disable-documentation.

[gbaz]

Agreed. I think this might be irritating the first time, but subsequently, as long as the versions don't change, then all those built haddocks will be cached and available, which on the whole will be handy. I'm certainly open to adding a global setting for controlling this as well though...

[andreasabel]

Does documentation: False in the cabal.project file work, @arybczak ?

[arybczak]

Power users can always supply --disable-documentation.

No, power users should be able to disable this in the configuration file.

might be irritating the first time, but subsequently, as long as the versions don't change

In reality they do almost every time you do cabal update.

Does documentation: False in the cabal.project file work, @arybczak ?

It doesn't. That's the crux of a problem. There is currently no way to override this behavior (other than passing --disable-documentation on the command line every single time).

Again, the problem here is not that this change is bad, it's that it's unconditionally enabled there is not way to disable it.

I see two potential solutions:

• Enabling documentation: True by default and reverting cabal haddock imply enable-documentation #8259.
• Providing another ~/.cabal/config option haddock-documentation with a default True that controls whether --enable-documentation is passed to cabal haddock so that it can be disabled.

[mouse07410]

A few naive questions.

1. Placing documentation: False in cabal.project should work, so, if it doesn't - it's probably a bug? And fixing it should resolve this issue?
2. Why would anybody invoke cabal haddock, if not for documentation?
3. If a user does not want documentation in general - why not set documentation: False in ~/.cabal/config, which presumably alleviates the problem?

[arybczak]

Placing documentation: False in cabal.project should work, so, if it doesn't - it's probably a bug?

Perhaps. But why should it work for cabal.project, but not for ~/.cabal/config?

Why would anybody invoke cabal haddock, if not for documentation?

Did you try running cabal haddock without --enable-documentation? You still get the haddock for your package, but without links to external packages. For some people (like me) it's sufficient.

[gbaz]

My understanding of the global level "documentation: False" is that it is supposed to control whether or not haddocks are run on packages as they are built in general. It is not supposed to determine if we recursively generate documentation for sub-dependencies of an individual package where we have already opted into asking for docs.

Again though, I think having a global opt-out of this flag would be a good thing, and a welcome PR. However, given the arguments both ways, circling around reverting the existing change seems like it won't be very fruitful.

[arybczak]

Again though, I think having a global opt-out of this flag would be a good thing, and a welcome PR.

Fair enough. I see there's a section haddock in ~/.cabal/config, I'll make a PR that adds a documentation setting to it.

[arybczak]

Ok, so I've spent a couple of hours on this and I think it's not that simple.

cabal/cabal-install/src/Distribution/Client/CmdHaddock.hs

Lines 146 to 148 in 04665d2

	installDoc = fromFlagOrDefault True (installDocumentation installFlags)
	flags' = flags { installFlags = installFlags { installDocumentation = Flag installDoc } }
	cliConfig = commandLineFlagsToProjectConfig globalFlags flags' mempty -- ClientInstallFlags, not needed here

installFlags at this point contains only flags set on the command line, so setting it to Flag means that it overwrites any setting in a configuration file (which IIUC is considered later).

I tried adding a haddock specific setting full-documentation:

diff --git a/Cabal/src/Distribution/Simple/Setup.hs b/Cabal/src/Distribution/Simple/Setup.hs
index 36f6aa22f..1e916d5f8 100644
--- a/Cabal/src/Distribution/Simple/Setup.hs
+++ b/Cabal/src/Distribution/Simple/Setup.hs
@@ -1364,6 +1364,7 @@ instance Parsec HaddockTarget where
 data HaddockFlags = HaddockFlags {
     haddockProgramPaths :: [(String, FilePath)],
     haddockProgramArgs  :: [(String, [String])],
+    haddockFullDocumentation :: Flag Bool,
     haddockHoogle       :: Flag Bool,
     haddockHtml         :: Flag Bool,
     haddockHtmlLocation :: Flag String,
@@ -1393,6 +1394,7 @@ defaultHaddockFlags :: HaddockFlags
 defaultHaddockFlags  = HaddockFlags {
     haddockProgramPaths = mempty,
     haddockProgramArgs  = [],
+    haddockFullDocumentation = Flag True,
     haddockHoogle       = Flag False,
     haddockHtml         = Flag False,
     haddockHtmlLocation = NoFlag,
@@ -1456,6 +1458,11 @@ haddockOptions showOrParseArgs =
    haddockKeepTempFiles (\b flags -> flags { haddockKeepTempFiles = b })
    trueArg
 
+  ,option "" ["full-documentation"]
+   "Generate documentation for dependencies"
+   haddockFullDocumentation (\v flags -> flags { haddockFullDocumentation = v })
+   trueArg
+
   ,option "" ["hoogle"]
    "Generate a hoogle database"
    haddockHoogle (\v flags -> flags { haddockHoogle = v })
@@ -1615,6 +1622,7 @@ data HaddockProjectFlags = HaddockProjectFlags {
 
     haddockProjectProgramPaths :: [(String, FilePath)],
     haddockProjectProgramArgs  :: [(String, [String])],
+    haddockProjectFullDocumentation :: Flag Bool,
     haddockProjectHoogle       :: Flag Bool,
     -- haddockHtml is not supported
     haddockProjectHtmlLocation :: Flag String,
@@ -1649,6 +1657,7 @@ defaultHaddockProjectFlags = HaddockProjectFlags {
     haddockProjectTestSuites   = Flag False,
     haddockProjectProgramPaths = mempty,
     haddockProjectProgramArgs  = mempty,
+    haddockProjectFullDocumentation = Flag True,
     haddockProjectHoogle       = Flag False,
     haddockProjectHtmlLocation = NoFlag,
     haddockProjectExecutables  = Flag False,
@@ -1729,6 +1738,11 @@ haddockProjectOptions _showOrParseArgs =
      haddockProjectGenContents (\v flags -> flags { haddockProjectGenContents = v})
      trueArg
 
+    ,option "" ["full-documentation"]
+     "Generate documentation for dependencies"
+     haddockProjectFullDocumentation (\v flags -> flags { haddockProjectFullDocumentation = v })
+     trueArg
+
     ,option "" ["hoogle"]
      "Generate a hoogle database"
      haddockProjectHoogle (\v flags -> flags { haddockProjectHoogle = v })
diff --git a/cabal-install/src/Distribution/Client/CmdHaddock.hs b/cabal-install/src/Distribution/Client/CmdHaddock.hs
index bfd2e8baf..64ce53ece 100644
--- a/cabal-install/src/Distribution/Client/CmdHaddock.hs
+++ b/cabal-install/src/Distribution/Client/CmdHaddock.hs
@@ -26,7 +26,7 @@ import Distribution.Client.TargetProblem
 import Distribution.Client.NixStyleOptions
          ( NixStyleFlags (..), nixStyleOptions, defaultNixStyleFlags )
 import Distribution.Client.Setup
-         ( GlobalFlags, ConfigFlags(..), InstallFlags (..))
+         ( GlobalFlags, ConfigFlags(..))
 import Distribution.Simple.Setup
          ( HaddockFlags(..), fromFlagOrDefault, trueArg )
 import Distribution.Simple.Command
@@ -143,9 +143,7 @@ haddockAction flags@NixStyleFlags {..} targetStrings globalFlags = do
     runProjectPostBuildPhase verbosity baseCtx buildCtx' buildOutcomes
   where
     verbosity = fromFlagOrDefault normal (configVerbosity configFlags)
-    installDoc = fromFlagOrDefault True (installDocumentation installFlags)
-    flags' = flags { installFlags = installFlags { installDocumentation = Flag installDoc } }
-    cliConfig = commandLineFlagsToProjectConfig globalFlags flags' mempty -- ClientInstallFlags, not needed here
+    cliConfig = commandLineFlagsToProjectConfig globalFlags flags mempty -- ClientInstallFlags, not needed here
 
 -- | This defines what a 'TargetSelector' means for the @haddock@ command.
 -- It selects the 'AvailableTarget's that the 'TargetSelector' refers to,
diff --git a/cabal-install/src/Distribution/Client/Config.hs b/cabal-install/src/Distribution/Client/Config.hs
index b829b51bd..9792f00d0 100644
--- a/cabal-install/src/Distribution/Client/Config.hs
+++ b/cabal-install/src/Distribution/Client/Config.hs
@@ -473,6 +473,7 @@ instance Semigroup SavedConfig where
         haddockProgramPaths  = lastNonEmpty haddockProgramPaths,
         -- TODO: NubListify
         haddockProgramArgs   = lastNonEmpty haddockProgramArgs,
+        haddockFullDocumentation = combine haddockFullDocumentation,
         haddockHoogle        = combine haddockHoogle,
         haddockHtml          = combine haddockHtml,
         haddockHtmlLocation  = combine haddockHtmlLocation,
diff --git a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs
index 7ed747fa9..bbe5971de 100644
--- a/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs
+++ b/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs
@@ -359,6 +359,7 @@ commandLineFlagsToProjectConfig globalFlags NixStyleFlags {..} clientInstallFlag
 
                                   -- Some flags to haddock should be passed to dependencies
                                   , packageConfigDocumentation = packageConfigDocumentation pc
+                                  , packageConfigHaddockFullDocumentation = packageConfigHaddockFullDocumentation pc
                                   , packageConfigHaddockHoogle = packageConfigHaddockHoogle pc
                                   , packageConfigHaddockHtml = packageConfigHaddockHtml pc
                                   , packageConfigHaddockInternal = packageConfigHaddockInternal pc
@@ -601,6 +602,7 @@ convertLegacyPerPackageFlags configFlags installFlags
     } = installFlags
 
     HaddockFlags {
+      haddockFullDocumentation  = packageConfigHaddockFullDocumentation,
       haddockHoogle             = packageConfigHaddockHoogle,
       haddockHtml               = packageConfigHaddockHtml,
       haddockHtmlLocation       = packageConfigHaddockHtmlLocation,
@@ -962,6 +964,7 @@ convertToLegacyPerPackageConfig PackageConfig {..} =
     haddockFlags = HaddockFlags {
       haddockProgramPaths  = mempty,
       haddockProgramArgs   = mempty,
+      haddockFullDocumentation = packageConfigHaddockFullDocumentation,
       haddockHoogle        = packageConfigHaddockHoogle,
       haddockHtml          = packageConfigHaddockHtml,
       haddockHtmlLocation  = packageConfigHaddockHtmlLocation,
@@ -1298,7 +1301,7 @@ legacyPackageConfigFieldDescrs =
           haddockForHackage (\v conf -> conf { haddockForHackage = v })
       ]
   . filterFields
-      [ "hoogle", "html", "html-location"
+      [ "full-documentation", "hoogle", "html", "html-location"
       , "foreign-libraries"
       , "executables", "tests", "benchmarks", "all", "internal", "css"
       , "hyperlink-source", "quickjump", "hscolour-css"
diff --git a/cabal-install/src/Distribution/Client/ProjectConfig/Types.hs b/cabal-install/src/Distribution/Client/ProjectConfig/Types.hs
index be3aae9bd..448dd42fc 100644
--- a/cabal-install/src/Distribution/Client/ProjectConfig/Types.hs
+++ b/cabal-install/src/Distribution/Client/ProjectConfig/Types.hs
@@ -274,6 +274,7 @@ data PackageConfig
        packageConfigRunTests            :: Flag Bool, --TODO: [required eventually] use this
        packageConfigDocumentation       :: Flag Bool, --TODO: [required eventually] use this
        -- Haddock options
+       packageConfigHaddockFullDocumentation :: Flag Bool, --TODO: [required eventually] use this
        packageConfigHaddockHoogle       :: Flag Bool, --TODO: [required eventually] use this
        packageConfigHaddockHtml         :: Flag Bool, --TODO: [required eventually] use this
        packageConfigHaddockHtmlLocation :: Flag String, --TODO: [required eventually] use this
diff --git a/cabal-install/src/Distribution/Client/ProjectPlanning.hs b/cabal-install/src/Distribution/Client/ProjectPlanning.hs
index 8d8947ae8..416dca002 100644
--- a/cabal-install/src/Distribution/Client/ProjectPlanning.hs
+++ b/cabal-install/src/Distribution/Client/ProjectPlanning.hs
@@ -1882,8 +1882,9 @@ elaborateInstallPlan verbosity platform compiler compilerprogdb pkgConfigDB
         elabReplTarget      = Nothing
         elabHaddockTargets  = []
 
-        elabBuildHaddocks   =
-          perPkgOptionFlag pkgid False packageConfigDocumentation
+        elabBuildHaddocks   = perPkgOptionFlag pkgid False packageConfigDocumentation
+          || elabHaddockFullDocumentation
+
 
         elabPkgSourceLocation = srcloc
         elabPkgSourceHash   = Map.lookup pkgid sourcePackageHashes
@@ -1959,7 +1960,7 @@ elaborateInstallPlan verbosity platform compiler compilerprogdb pkgConfigDB
         elabProgPrefix          = perPkgOptionMaybe pkgid packageConfigProgPrefix
         elabProgSuffix          = perPkgOptionMaybe pkgid packageConfigProgSuffix
 
-
+        elabHaddockFullDocumentation = perPkgOptionFlag pkgid False packageConfigHaddockFullDocumentation
         elabHaddockHoogle       = perPkgOptionFlag pkgid False packageConfigHaddockHoogle
         elabHaddockHtml         = perPkgOptionFlag pkgid False packageConfigHaddockHtml
         elabHaddockHtmlLocation = perPkgOptionMaybe pkgid packageConfigHaddockHtmlLocation
@@ -3770,6 +3771,7 @@ setupHsHaddockFlags (ElaboratedConfiguredPackage{..}) (ElaboratedSharedConfig{..
           Just prg -> [( programName haddockProgram
                        , locationPath (programLocation prg) )],
       haddockProgramArgs   = mempty, --unused, set at configure time
+      haddockFullDocumentation = toFlag elabHaddockFullDocumentation,
       haddockHoogle        = toFlag elabHaddockHoogle,
       haddockHtml          = toFlag elabHaddockHtml,
       haddockHtmlLocation  = maybe mempty toFlag elabHaddockHtmlLocation,
diff --git a/cabal-install/src/Distribution/Client/ProjectPlanning/Types.hs b/cabal-install/src/Distribution/Client/ProjectPlanning/Types.hs
index bda338897..05400bbf8 100644
--- a/cabal-install/src/Distribution/Client/ProjectPlanning/Types.hs
+++ b/cabal-install/src/Distribution/Client/ProjectPlanning/Types.hs
@@ -278,6 +278,7 @@ data ElaboratedConfiguredPackage
 
        elabInstallDirs           :: InstallDirs.InstallDirs FilePath,
 
+       elabHaddockFullDocumentation :: Bool,
        elabHaddockHoogle         :: Bool,
        elabHaddockHtml           :: Bool,
        elabHaddockHtmlLocation   :: Maybe String,
diff --git a/cabal-install/src/Distribution/Client/Setup.hs b/cabal-install/src/Distribution/Client/Setup.hs
index 6db91d9cf..9ce34b41e 100644
--- a/cabal-install/src/Distribution/Client/Setup.hs
+++ b/cabal-install/src/Distribution/Client/Setup.hs
@@ -1776,7 +1776,7 @@ haddockOptions showOrParseArgs
                           | descr <- optionDescr opt] }
     | opt <- commandOptions Cabal.haddockCommand showOrParseArgs
     , let name = optionName opt
-    , name `elem` ["hoogle", "html", "html-location"
+    , name `elem` ["full-documentation", "hoogle", "html", "html-location"
                   ,"executables", "tests", "benchmarks", "all", "internal", "css"
                   ,"hyperlink-source", "quickjump", "hscolour-css"
                   ,"contents-location", "use-index", "for-hackage", "base-url", "lib"]
diff --git a/cabal-install/tests/UnitTests/Distribution/Client/ProjectConfig.hs b/cabal-install/tests/UnitTests/Distribution/Client/ProjectConfig.hs
index 94f419088..849b0cbe4 100644
--- a/cabal-install/tests/UnitTests/Distribution/Client/ProjectConfig.hs
+++ b/cabal-install/tests/UnitTests/Distribution/Client/ProjectConfig.hs
@@ -565,7 +565,7 @@ instance Arbitrary PackageConfig where
         <*> arbitrary <*> arbitrary
         <*> arbitrary <*> arbitrary
         <*> arbitrary <*> arbitrary <*> arbitrary
-        <*> arbitrary <*> arbitrary
+        <*> arbitrary <*> arbitrary <*> arbitrary
         <*> arbitraryFlag arbitraryShortToken
         <*> arbitrary
         <*> arbitrary
@@ -629,6 +629,7 @@ instance Arbitrary PackageConfig where
                          , packageConfigDumpBuildInfo = x27_1
                          , packageConfigRunTests = x28
                          , packageConfigDocumentation = x29
+                         , packageConfigHaddockFullDocumentation = x30_0
                          , packageConfigHaddockHoogle = x30
                          , packageConfigHaddockHtml = x31
                          , packageConfigHaddockHtmlLocation = x32
@@ -689,6 +690,7 @@ instance Arbitrary PackageConfig where
                       , packageConfigDumpBuildInfo = x27_1'
                       , packageConfigRunTests = x28'
                       , packageConfigDocumentation = x29'
+                      , packageConfigHaddockFullDocumentation = x30_0'
                       , packageConfigHaddockHoogle = x30'
                       , packageConfigHaddockHtml = x31'
                       , packageConfigHaddockHtmlLocation = x32'
@@ -720,7 +722,7 @@ instance Arbitrary PackageConfig where
           (x15', x16', x53', x17', x18', x19')),
          ((x20', x20_1', x21', x22', x23', x24'),
           (x25', x26', x27', x27_1', x28', x29'),
-          (x30', x31', x32', (x33', x33_1'), x34'),
+          (x30_0', x30', x31', x32', (x33', x33_1'), x34'),
           (x35', x36', x37', x38', x43', x39'),
           (x40', x41'),
           (x44', x45', x46', x47', x48', x49', x51', x52', x54', x55'),
@@ -735,7 +737,7 @@ instance Arbitrary PackageConfig where
                   x19)),
                ((x20, x20_1, x21, x22, x23, x24),
                  (x25, x26, x27, x27_1, x28, x29),
-                 (x30, x31, x32, (x33, x33_1), x34),
+                 (x30_0, x30, x31, x32, (x33, x33_1), x34),
                  (x35, x36, fmap NonEmpty x37, x38, x43, fmap NonEmpty x39),
                  (x40, x41),
                  (x44, x45, x46, x47, x48, x49, x51, x52, x54, x55), x56))
-- 
2.39.2

So that whether haddocks for dependencies are installed is done in elaborateInstallPlan, where global configuration is also considered.

The problem then becomes twofold:

1. Weird interaction between these flags, e.g. what should --disable-documentation --haddock-full-documentation mean?
2. Unfortunately it looks like haddock options are considered for all commands, not only the haddock action. So setting full-documentation in the haddock section to True amounts to the same thing as setting documentation to True 😞

I don't know how to solve these issues at the moment and I don't have more time to spend on this, but I'll restate again that the patch that added this goes against anything else done in cabal, i.e. that things are configurable via files.

I'll echo what @phadej said here: if people want full haddocks, they should set documentation to True.

If you decide to keep this as is, oh well, I suppose I'll live with running a fork. Presumably there will be many more people complaining about this after the release as I suspect not many people tried 3.9.0.0.

[Mikolaj]

@arybczak: thank you for the investigation. We are about to release, so let's gather here further feedback of this feature and ideas for improvements.

[vdukhovni]

FWIW, I strongly agree that the command-line override needs a configuration file equivalent.
As to whether dependency haddocks should be on by default, my inclination is to say "no".

[dcoutts]

Perhaps a nice solution (though not an easy solution to implement) would be if we could make the haddocs independently installable: so that building the docs for a dependency did not imply rebuilding the code. For example, similar to how we handle building profiling these days. Such an approach may well mean the docs have to be independent entries in the store and in the build plan, rather than being "part of" each entry in the plan.

[michaelpj]

I made another issue for the problem of possibly building the docs separately: #10111