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

haskell_cabal_library: Add haddocks #1102

Merged
merged 1 commit into from
Dec 5, 2019
Merged

Conversation

thufschmitt
Copy link
Contributor

Build and export the haddocks for haskell_cabal_libraryes

This builds on top of #1097 because the cabal_wrapper part would have to
be rewritten otherwise (though it's minor and easy to port to master if
#1097 gets delayed)

@thufschmitt thufschmitt force-pushed the cabal_library_haddock branch 2 times, most recently from ce3fa9d to b3745d1 Compare October 11, 2019 08:35
@Profpatsch
Copy link
Contributor

Rebased on master.

@thufschmitt
Copy link
Contributor Author

@Profpatsch I've fixed the failure. Waiting for the CI to confirm, but it should be good now

@Profpatsch
Copy link
Contributor

@regnat do you still want to merge this?

@thufschmitt
Copy link
Contributor Author

Well that's up to you ;)

I just raised the fact that it makes the build of the cabal packages slower. But if nobody's stepping against it then yes, please merge

@mboes
Copy link
Member

mboes commented Nov 18, 2019

Haddock does add a fair amount of time to build times. Is there some way to make this optional?

@thufschmitt
Copy link
Contributor Author

Haddock does add a fair amount of time to build times. Is there some way to make this optional?

I don't think we can't make it like haskell_library where haddocks are built on-demand (because all the build is happening in a single bazel action and I'm not sure we can reasonably split this as that would mean messing up with cabal's internal buisness). But we can add an extra argument to haskell_cabal_library to indicate whether or not we want the haddocks to be built − at the cost of a bit of complexity in the code as this has some repercussions both in the build script and in the exported providers

@mboes
Copy link
Member

mboes commented Nov 27, 2019

mmh, let's start by merging this. Provided we have an idea of the impact. Could you collect numbers about how much slower bazel build @stackage//stack is?

@thufschmitt
Copy link
Contributor Author

So I benchmarked building @stackage//:proto-lens (because stack isn't exported in rules_haskell) both on this branch and on master, and strangely enough it's notably faster on this branch:

Branch Mean [s] Min [s] Max [s] Relative
haskell_cabal_library 135.147 ± 4.914 131.490 148.002 1.0
master 186.213 ± 3.220 183.164 193.916 1.00

I guess there has been another change on master making the build slower, I'll see whether I get the same results after a rebase

@thufschmitt
Copy link
Contributor Author

(ftr the above results have been generated with hyperfine --export-markdown "benchs_on_branch_$(git rev-parse --abbrev-ref HEAD).md" -p "bazel clean --expunge && rm -rf ~/.cache/bazel_disk_cache" "bazel build @stackage//:proto-lens")

@thufschmitt
Copy link
Contributor Author

I did some new benchmarks after rebasing on top of master, and indeed we now have a slight slowdown on this branch:

Branch Mean [s] Min [s] Max [s] Relative
haskell_cabal_library (rebased) 199.437 ± 3.021 196.493 206.374 1.00
master 186.213 ± 3.220 183.164 193.916 1.00

So this makes a 7% slowdown on this example, which I think is acceptable

(on a side note, I'd like to know why master seems to be much slower than what it was some times ago. Maybe that's because of #1156 )

Build and export the haddocks for `haskell_cabal_library`es
@mboes mboes added the merge-queue merge on green CI label Dec 5, 2019
@mergify mergify bot merged commit 3659373 into master Dec 5, 2019
@mergify mergify bot deleted the cabal_library_haddock branch December 5, 2019 15:58
@mergify mergify bot removed the merge-queue merge on green CI label Dec 5, 2019
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.

3 participants