Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build haddocks for dependencies (#143) #362

Merged
merged 2 commits into from
Jun 21, 2015
Merged

Build haddocks for dependencies (#143) #362

merged 2 commits into from
Jun 21, 2015

Conversation

borsboom
Copy link
Contributor

This doesn't yet generate the index or do relative links, and it requires rebuilding the package in order to generate haddocks, but does work well in my testing and think it's worth merging with the current feature set (especially as this touches enough code that conflicts are frequent).

As for building haddocks without rebuilding the packages: this should be doable by adding some extra logic to singleBuild that lets it skip runhaskell Setup.hs build when building only haddocks (along with some adjustments to ConfigCache and maybe getInstalled), but interested in opinion about whether that's the right approach or if this would be better done at the Plan level (i.e. split the plan into build, haddock, and install steps). Note that haddocks need to be built before installing the package (that way, install takes care of putting them where they belong).

For #143.

@snoyberg
Copy link
Contributor

Will review tomorrow.

@snoyberg
Copy link
Contributor

I merged with master and ran all of the tests locally, which passed. Pushing that merge back onto the 143-haddock-deps branch.

@@ -593,6 +596,16 @@ singleBuild ac@ActionContext {..} ee@ExecuteEnv {..} task@Task {..} =
TTLocal lp -> "build" : map T.unpack (Set.toList $ lpComponents lp)
TTUpstream _ _ -> ["build"]

when (shouldBuildHaddock eeBuildOpts eeWanted (packageName package) &&
packageHasLibrary package &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this redundant with packageHasExposedModules? If not, under what circumstances may it be necessary? If it is redundant, looks like a good place to use assert.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure it's redundant (I think I wrote this before I added the packageHasExposedModules check to shouldBuildHaddock).

snoyberg added a commit that referenced this pull request Jun 21, 2015
@snoyberg snoyberg merged commit b90df3d into master Jun 21, 2015
@snoyberg snoyberg deleted the 143-haddock-deps branch June 21, 2015 04:55
@snoyberg
Copy link
Contributor

LGTM, nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants