Add Haddock flags to package hash

[alexbiehl]

This should help with #5341

This adds haddock flags and arguments to the package hash. Changing haddock flags for non-local packages will retrigger a build of these.

I view this as a quick fix. I think we really want documentation to be treated as a separate component.

[RyanGlScott]

Very cool, thanks @alexbiehl! I tested this out on the package in #5341 (comment) and it works quite well for my use case.

I have one question: once this change is in place, will it still be possible to build a package's Haddocks without having to build the Haddocks for all of its libraries? The use case for this feature would be something like haskell-ci, where it would be convenient to quickly test that a package's Haddocks work without needing to rebuild all of the dependencies anew. (If the answer is "no", I don't think that should prevent this patch from being landed—I'm just curious.)

[alexbiehl]

I ran out of time to test unfortunately but my understanding is that passing --enable-documentation should only haddock local packages. Could you verify?

If it doesn't we should fix that.

[RyanGlScott]

I ran out of time to test unfortunately but my understanding is that passing --enable-documentation should only haddock local packages. Could you verify?

This wasn't my experience. Running cabal new-build --enable-documentation caused all of my package's dependencies to be rebuilt with documentation enabled.

[alexbiehl]

Ok. I will see what I can do. Meanwhile on my local machine:

Trying to build cabal with this patch enbled:

$ ./cabal-dev new-build exe:cabal
Build profile: -w ghc-8.4.2 -O1
In order, the following will be built (use -v for more details):
 - Cabal-2.3.0.0 (lib) (first run)
 - base16-bytestring-0.1.1.6 (lib:base16-bytestring) (requires build)
 - base64-bytestring-1.0.0.1 (lib) (requires build)
 - echo-0.1.3 (lib) (requires build)
 - clock-0.7.2 (lib) (requires build)
 - call-stack-0.1.0 (lib) (requires build)
 - colour-2.3.4 (lib) (requires build)
 - cryptohash-sha256-0.11.101.0 (lib) (requires build)
 - ed25519-0.0.5.0 (lib) (requires build)
Assertion failed
CallStack (from HasCallStack):
  assert, called at ./Distribution/Client/ProjectPlanning.hs:236:5 in cabal-install-2.3.0.0-inplace:Distribution.Client.ProjectPlanning

I will investigate here.

[RyanGlScott]

Out of curiosity, if --enable-documentation alone is intended to just be for local packages, how do you envision someone specifying that the documentation should be built for all packages?

[alexbiehl]

I have a small time frame left, I will write something up in a minute hopefully. Ryan Scott <[email protected]> schrieb am So., 24. Juni 2018, 19:48:

…

Out of curiosity, if --enable-documentation alone is intended to just be for local packages, how do you envision someone specifying that the documentation should be built for *all* packages? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5395 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AByiiTZvgKmGahMd4Hg_kX3J2jgsuTYUks5t_9EFgaJpZM4U1HFK> .

[alexbiehl]

@hvr @RyanGlScott @23Skidoo Here is a (hopefully complete) list on how to pass documentation: true to cabal and what packages it should consider for documentation generation:

• Global configuration $HOME/.cabal/config

  documentation: true
  

  • local
  • non local

• CLI argument--enable-documentation

  • local
  • non local

• Project local configuration cabal.project / cabal.project.local

   documentation: true
  

  • local
  • non local

• Project local configuration cabal.project / cabal.project.local

  package *
    documentation: true 
  

  (I think cabal new-configure --enable-documentation sets this)

  • local
  • non local

Does it make sense/is it what we want?

Once we agree what packages should be affected when I can make cabal compliant.

[hvr]

Fwiw, an invariant that ought to hold is:

• cabal new-configure --enable-documentation; cabal new-build ought to have the same effect as cabal new-build --enable-documentation

• And when we cabal new-haddock --haddock-for-hackage it should ideally imply --enable-documentation on local and non-local packages in the current install-plan. (Because for uploading to Hackage we need everything properly linked)

@RyanGlScott btw, for the purpose of haskell-ci, why do you think that having --enable-documentation apply to non-local packages is a bad thing (I specifically am confused about the "rebuild everything" part... wouldn't those be cached just like the no-documentation artifacts)?

[RyanGlScott]

@RyanGlScott btw, for the purpose of haskell-ci, why do you think that having --enable-documentation apply to non-local packages is a bad thing

For the same reason that I wouldn't want haskell-ci to build all the test suite and benchmark components of all downstream dependencies: they're irrelevant to ensuring that the package you actually care about builds correctly, and building them simply wastes cycles.

(I specifically am confused about the "rebuild everything" part... wouldn't those be cached just like the no-documentation artifacts)?

Caching exists, but it doesn't change the fact that on a from-scratch build, you're now going to have to build every single dependency twice. For large projects like lens, this can add up to the order of 10–15 extra minutes.

[hvr]

@RyanGlScott I'm still confused about the need to compile things twice during CI -- just build everything always (& once) w/ enable-docs?

However, what's the purpose of building haddocks during haskell-ci to begin with, if you're only building partial docs? isn't the intent of buidling haddocks as part of CI in order to validate that all haddock-links are valid (which requires having transitive deps being haddocked as well)?

[RyanGlScott]

@RyanGlScott I'm still confused about the need to compile things twice during CI -- just build everything always (& once) w/ enable-docs?

Ah, never mind. For some reason, my mind was stuck in the old paradigm of building the package, then building documentation separately. I forgot that you could just enable documentation: true from the get-go and have that be on when you're building dependencies, eliminating the need to build everything twice.

In that case, please ignore my ramblings above.

[merijn]

The downside of this PR seems to be that if I build a package, then rebuild it with --enable-documentation then my global store will include all my dependencies twice.

[hvr]

@merijn that's correct; the ideal solution would be to implement "lazy haddocks" which would require to somehow model haddocks into either a companion-component or some other hack (and fwiw, Hi Haddock may provide us with a totally different approach to this issue). That, however, might require more time and effort to achieve, and in the meantime we'd be stuck with the current stateful situation that the nix-style does not track haddocks resulting in an imho even worse user experience, as you'd still end up requiring to rebuild everything if you realise you need the docs, except it's not being tracked in the nix-style hash -- current situation is worst of both worlds IMO, and this PR at least makes the situation more Nix-principled...

[merijn]

@alexbiehl also double check how things interact with https://github.com/haskell/cabal/blob/master/Cabal/Distribution/Simple/Program/Builtin.hs#L319

When I was asking which flags to filter I was initially told haddock flags shouldn't affect the hash, so that filters them out. It's called from

cabal/cabal-install/Distribution/Client/ProjectPlanning/Types.hs

Line 317 in 2ce4858

normaliseConfiguredPackage :: ElaboratedSharedConfig

which in turns is called by

cabal/cabal-install/Distribution/Client/ProjectBuilding.hs

Lines 334 to 359 in 2ce4858

	newPackageFileMonitor :: ElaboratedSharedConfig
	-> DistDirLayout
	-> DistDirParams
	-> PackageFileMonitor
	newPackageFileMonitor shared
	DistDirLayout{distPackageCacheFile}
	dparams =
	PackageFileMonitor {
	pkgFileMonitorConfig =
	FileMonitor {
	fileMonitorCacheFile = distPackageCacheFile dparams "config",
	fileMonitorKeyValid = (==) `on` normaliseConfiguredPackage shared,
	fileMonitorCheckIfOnlyValueChanged = False
	},
	pkgFileMonitorBuild =
	FileMonitor {
	fileMonitorCacheFile = distPackageCacheFile dparams "build",
	fileMonitorKeyValid = \componentsToBuild componentsAlreadyBuilt ->
	componentsToBuild `Set.isSubsetOf` componentsAlreadyBuilt,
	fileMonitorCheckIfOnlyValueChanged = True
	},
	pkgFileMonitorReg =
	newFileMonitor (distPackageCacheFile dparams "registration")
	}

(for determining whether to rebuild local dependencies) and

cabal/cabal-install/Distribution/Client/ProjectPlanning.hs

Lines 3564 to 3593 in 2ce4858

	packageHashConfigInputs shared@ElaboratedSharedConfig{..} pkg =
	PackageHashConfigInputs {
	pkgHashCompilerId = compilerId pkgConfigCompiler,
	pkgHashPlatform = pkgConfigPlatform,
	pkgHashFlagAssignment = elabFlagAssignment,
	pkgHashConfigureScriptArgs = elabConfigureScriptArgs,
	pkgHashVanillaLib = elabVanillaLib,
	pkgHashSharedLib = elabSharedLib,
	pkgHashDynExe = elabDynExe,
	pkgHashGHCiLib = elabGHCiLib,
	pkgHashProfLib = elabProfLib,
	pkgHashProfExe = elabProfExe,
	pkgHashProfLibDetail = elabProfLibDetail,
	pkgHashProfExeDetail = elabProfExeDetail,
	pkgHashCoverage = elabCoverage,
	pkgHashOptimization = elabOptimization,
	pkgHashSplitSections = elabSplitSections,
	pkgHashSplitObjs = elabSplitObjs,
	pkgHashStripLibs = elabStripLibs,
	pkgHashStripExes = elabStripExes,
	pkgHashDebugInfo = elabDebugInfo,
	pkgHashProgramArgs = elabProgramArgs,
	pkgHashExtraLibDirs = elabExtraLibDirs,
	pkgHashExtraFrameworkDirs = elabExtraFrameworkDirs,
	pkgHashExtraIncludeDirs = elabExtraIncludeDirs,
	pkgHashProgPrefix = elabProgPrefix,
	pkgHashProgSuffix = elabProgSuffix
	}
	where
	ElaboratedConfiguredPackage{..} = normaliseConfiguredPackage shared pkg

(to compute the package hash for non-local dependencies).

[hvr]

@merijn

When I was asking which flags to filter I was initially told haddock flags shouldn't affect the hash, so that filters them out.

Indeed, the rationale was according to the rule-of-thumb, that they wouldn't affect the compiled .o artefacts (and thus affect the primary goal of compiled programs). So consider including them now more of a pragmatic choice until we have a different way to address the problem that haddocks can't be generated lazily yet, and so we need to consider them (for now) part of the nix-style hash in order to improve the user experience until we have a different more granular way to represent haddocks as artefacts.

[phadej]

@bgamari is asking for a release branch, @23Skidoo said this should go in. What's the state of this PR?

Ping @alexbiehl.

@merijn could you verify that interactions are ok?

[alexbiehl]

This is fine to my knowledge. @merijn, @hvr and @sjakobi dogfooded. I haven't heard negative things (apart from the obvious store trashing).

[alexbiehl]

Ugh. I just saw #5395 (comment). @merijn is this still a thing? Should I update my patch to remove haddock flags from your filter?

[merijn]

I didn't actually touch the filtering I added for Haddock flags, on account of being to busy. It looks like @alexbiehl added custom fields to the package hash, but I'm not sure if anyone looked at the interaction? For safety's sake my change in Cabal should probably be reverted? This should be simple enough to do, the relevant line is: https://github.com/haskell/cabal/blob/master/Cabal/Distribution/Simple/Program/Builtin.hs#L319

This function just has to be changed to return it's 3rd argument (the list of args) unchanged, so \_ _ xs -> xs. So should be trivial for @alexbiehl to include in his patches.

[merijn]

(Yay for proper design resulting in only a single trivial change to avoid any problems! ;))

[23Skidoo]

Merged, thanks! Sorry for taking so long.