Hadrian: Misc. fixes in Haddock rules
ClosedPublic

Authored by harpocrates on Nov 9 2018, 1:51 PM.

Details

Summary
  • Pass 'GHC/Prim.hs' to Haddock when processing 'ghc-prim'. This file is autogenerated for the sole purpose of giving Haddock something to process, so we really should make sure it gets through to Haddock!
  • Add a "docs-haddock" build rule, which should build all Haddock docs that the Makefile builds by default (all libs + index for libs + ghc)
  • Prune some unnecessary rules (esp. gen_contents_index)

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
harpocrates created this revision.Nov 9 2018, 1:51 PM
harpocrates edited the summary of this revision. (Show Details)Nov 9 2018, 1:53 PM
snowleopard requested changes to this revision.Nov 13 2018, 5:41 PM

@harpocrates Many thanks! Apologies for taking so long to review. I've left a few minor comments, mostly just trying to make this patch a bit simpler.

hadrian/src/Expression.hs
82 ↗(On Diff #18636)

This appears to be unused so far, right? Perhaps, it's worth postponing adding this predicate until it actually becomes useful?

hadrian/src/Rules/Documentation.hs
74

I am a bit confused: judging by the above comment, we build all Haddocks as part of the docs rule. But then why couldn't we simplify docs by calling the new rule haddocks from it?

We also need to add this rule to documentation: to the README file and the Hadrian wiki page https://ghc.haskell.org/trac/ghc/wiki/Building/Hadrian/QuickStart.

To make rules a bit more uniform, I also suggest renaming this rule to docs-haddock: we will follow this convention when adding other documentation rules.

77

Let's make this a bit more direct by inlining -/-:

root -/- htmlRoot -/- "libraries/ghc/ghc.haddock"

hadrian/src/Rules/Generate.hs
54

This feels overly indirect. Why not just ghcPrimDependency "GHC/Prim.hs"?

Furthermore, I don't like excessive indirection where we introduce a new notion haddockExtraSources, which in reality is fixed simply to GHC/Prim.hs. Why not inline this at the call site? This will make it easier to understand.

This revision now requires changes to proceed.Nov 13 2018, 5:41 PM

Thanks for the review! I'll address the feedback in the coming day. In the meantime, I have a couple followup questions...

Additionally, I noticed that there Hadrian doesn't generate the short wrapper shell script the make-based system used to produce at ./inplace/bin/haddock (which did not much more than call the actual haddock binary with the right -B and -l arguments). Is this intentional? It makes calling the Haddock built by Hadrian a bit more tedious...

hadrian/src/Expression.hs
82 ↗(On Diff #18636)

Are you sure about postponing this?

As far as I'm concerned, it is already useful. It's been my replacement for the old mk/build.mk's EXTRA_HADDOCK_OPTS variable. Besides, it is in good company:

instance BuilderPredicate a => BuilderPredicate (Stage -> a)
instance BuilderPredicate a => BuilderPredicate (FilePath -> a)
instance BuilderPredicate a => BuilderPredicate (CcMode -> a)
instance BuilderPredicate a => BuilderPredicate (GhcMode -> a)

I'll leave the final decision up to you though.

hadrian/src/Rules/Documentation.hs
74

I am a bit confused: judging by the above comment, we build all Haddocks as part of the docs rule. But then why couldn't we simplify docs by calling the new rule haddocks from it?

How would I do this? I'm still wrapping my head around all of the shake/hadrian operators/types so I'm probably missing something obvious. For instance, although it must be happening somewhere, I can't see figure out how the docs rule ends up "need"-ing root -/- htmlRoot -/- "libraries/index.html". I don't see a straightforward refactor of docs that uses haddocks.

Additionally, I noticed that there Hadrian doesn't generate the short wrapper shell script the make-based system used to produce

Indeed. Hadrian used to build wrapper scripts, but at some point this functionality was removed as part of restructuring the build directory structure. My understanding is that the wrapper scripts will be returned at some point. @alpmestan Could you please clarify this? Is there a ticket somewhere where we track the future of wrapper scripts?

It makes calling the Haddock built by Hadrian a bit more tedious...

Just to clarify: do you mean you need to provide additional -B and -l arguments to Haddock when calling it manually? Or are there any other complications? In any case, I think this deserves a new GHC ticket, unless @alpmestan points us to an existing one.

hadrian/src/Expression.hs
82 ↗(On Diff #18636)

My point is: you say this feature is already useful, but you don't use it in this patch :)

Why don't we add it together with an immediate use case for it?

The example with EXTRA_HADDOCK_OPTS you give does not seem right, because EXTRA_HADDOCK_OPTS corresponds to using Haddock in the mode BuildPackage, so it's equivalent is builder (Haddock BuildPackage). Using the predicate builder Haddock would be incorrect in this case, because it would also be true in the mode BuildIndex, but EXTRA_HADDOCK_OPTS appear to be not used when building the index.

hadrian/src/Rules/Documentation.hs
74

For instance, although it must be happening somewhere, I can't see figure out how the docs rule ends up "need"-ing root -/- htmlRoot -/- "libraries/index.html".

This is exactly the source of my confusion too. There should be an overlap between these build rules, but it's currently not visible. Perhaps, we need to refactor the docs rule to make the overlap apparent? Or maybe docs is actually incorrect and is missing some dependencies?

This needs to be figured out, as otherwise future Hadrian developers will be confused too.

As a first step, perhaps you can add a Note a describing how the docs and haddocks rules work, i.e. which files they explicitly need and which files get build as a result?

snowleopard added inline comments.Nov 14 2018, 5:45 AM
hadrian/src/Expression.hs
82 ↗(On Diff #18636)

Looking at the module Settings.Builders.Haddock where most Haddock arguments live, I can see that there is some overlap between their argument lists, specifically:

, arg $ "-B" ++ root -/- "stage1" -/- "lib"
, arg $ "--lib=" ++ root -/- "docs"

which is not a lot. Are there other examples of arguments that should be passed to Haddock regardless of the execution mode? As I mentioned, EXTRA_HADDOCK_OPTS doesn't seem like the right example.

harpocrates added inline comments.Nov 14 2018, 10:59 AM
hadrian/src/Expression.hs
82 ↗(On Diff #18636)

I had not realized EXTRA_HADDOCK_OPTS was only builder (Haddock BuildPackage) - thanks for pointing that out! I guess this instance isn't really all that interesting.

There are other arguments that should be would be passed to Haddock regardless of the execution mode (so would benefit from such an instance), but they are mostly things that only people debugging Haddock would care about: --css, --theme, --built-in-themes, --mathjax, --pretty-html, --title, --no-warnings, --no-print-missing-docs, and maybe a couple others.

  • Restructure GHC.Prim case for Haddock
  • Remove unused gen_contents_index/prologue rules
  • Simplify some rules & add comments
harpocrates edited the summary of this revision. (Show Details)Nov 20 2018, 3:07 AM
harpocrates marked 5 inline comments as done.Nov 20 2018, 3:20 AM

I think I now fully understand the rules here. I wasn't able to find a clean way for the docs rule to call the docs-haddock rule, but hopefully the comments I left will make it clear what is happening.

Additionally, it appears that the rules involving gen_contents_index and libraries/prologue.txt are useless: the functionality in gen_contents_index is now expressed by Haddock BuildIndex builder and libraries/prologue.txt is anyways referenced directly (so there is no point copying it into the build directory).

Regarding the case of the missing wrapper scripts: Moritz mentioned on IRC that these were removed as part of an effort to make the build output completely relocatable. He apparently put some work into making the GHC wrapper scripts obsolete so I think the right thing is for me to see what I can do to port similar logic over to Haddock. I'll do that in a separate patch - most of the work should be Haddock-side.

@harpocrates Thank you! The newly added comments and simplifications to the docs rule are very helpful.

Also thanks a lot for your further work on Haddock, it would be great to get rid of all wrappers.

I left one further minor nit-pick about union. Happy for this to be merged.

hadrian/src/Rules/Documentation.hs
167

I believe there should be no duplicates in this case, so ++ will be equivalent.

In general, I suggest not to use Data.List.union on lists of files, because it takes quadratic time (the lists are not sorted). Although we are unlikely to ever hit a real performance issue, this just makes the reader wonder why we have union here and ++ everywhere else.

Shake is efficient enough when dealing with duplicates in dependencies.

harpocrates added inline comments.Nov 20 2018, 2:53 PM
hadrian/src/Rules/Documentation.hs
167

There are duplicates: GHC/PrimopWrappers.hs is in generatedSrcs and in vanillaSrcs. Shake may be smart enough to deal with duplicates, but Haddock isn't, and srcs ends up being passed as an argument to Haddock.

In this case, union is actually faster since the generatedSrcs is always very small. Things would be quite different if the arguments to union were flipped... However, ask again and I'll switch this to Data.Set.toList (Data.Set.union (Data.Set.fromList vanillaSrcs) (Data.Set.fromList generatedSrcs)). 😄

snowleopard added inline comments.Nov 20 2018, 2:57 PM
hadrian/src/Rules/Documentation.hs
167

Ah, I see! Thanks for clarifying -- I didn't realise two things: 1) that there are indeed duplicates, and 2) that this list is also passed to Haddock who can't deal with them! This is non-obvious, so let's add a comment? Happy to stick to union in this case.

One more thing: we'd like to follow a convention that all Hadrian-only commits are prefixed with Hadrian: . I guess renaming this diff should be sufficient for it to land with the right commit message.

harpocrates edited the summary of this revision. (Show Details)
  • Better comment around union
harpocrates retitled this revision from Misc. fixes for Hadrian's handling of Haddock to Hadrian: Misc. fixes for Hadrian's handling of Haddock.Nov 20 2018, 3:12 PM
harpocrates retitled this revision from Hadrian: Misc. fixes for Hadrian's handling of Haddock to Hadrian: Misc. fixes in Haddock rules.Nov 20 2018, 3:13 PM
snowleopard accepted this revision.EditedNov 20 2018, 3:27 PM

Thank you! I believe this can now be merged.

This revision is now accepted and ready to land.Nov 20 2018, 3:27 PM

Looks good to me!

Regarding wrapper scripts, here's the summary of the plan that we've been floating around: https://ghc.haskell.org/trac/ghc/ticket/15925

This revision was automatically updated to reflect the committed changes.