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

An option to list packages ignored by haskell_doc #336

Closed
wants to merge 1 commit into from

Conversation

guibou
Copy link
Contributor

@guibou guibou commented Jul 14, 2018

Fix #335 by providing haddock_ignore_prebuilts attribute to haskell_toolchain.

`haskell_doc` will ignore the documentation of all the prebuilt
package listed in `haddock_ignore_prebuilts`.
@guibou guibou force-pushed the guibou/doc_ignore_deps branch from c44e777 to df74df5 Compare July 14, 2018 20:34
@guibou guibou changed the title Add test for haskell_doc when target dependency have no documentation An option to list packages ignored by haskell_doc Jul 14, 2018
@guibou guibou requested review from mrkkrp and mboes July 14, 2018 20:37
@mboes
Copy link
Member

mboes commented Jul 15, 2018

This is very ad hoc information for a toolchain to track. Could you tell us more about,

  1. the specific scenario in which you originally encountered haskell_doc fails if sub-package does not provide haddock file #335,
  2. the alternatives you considered how they (did not) work out.

@guibou
Copy link
Contributor Author

guibou commented Jul 15, 2018

  1. I work on a big monorepo with hundreds of prebuilt dependencies provided by nixpkgs and rules_nixpkgs. A few of theses dependencies have a broken haddock (for reasons which may depend on the package, nix, bazel, or rules_haskell), so it is impossible to use haskell_doc. Everything can be fixed upstream (or with local overrides), but that's a lot more work than this ad hoc solution.

For example, consider a developer, with no knowledge of nix, bazel or haddock, who just add a new dependency to one of his haskell_library, this may break the documentation for all the repository and his only solution is to fix the documentation of a package he doesn't own using a tool he may not know about (nix). That's a lot to ask for a developer who may have other constraints in his schedules. This PR provides a perfectible, but quick and easy solution.

  1. Another solution may be to let haddock fails gracefully and ignore the broken package:
  • inside haddock_wrapper.sh, loop over the haddock call and progressively removes failing package until it converges to a working solution. That's fragile but may work.
  • inside the haskell_doc rule, test for the presence of the package.haddock file for each sub package.

@mboes
Copy link
Member

mboes commented Jul 15, 2018

A few of theses dependencies have a broken haddock (for reasons which may depend on the package, nix, bazel, or rules_haskell).

I understand. I'm asking for specific instances. From the test in the PR, should I infer mtl is one such package? And if so, is this because of a nixpkgs packaging issue?

@mrkkrp
Copy link
Member

mrkkrp commented Jul 15, 2018

IMO, if nix packages things incorrectly that should be fixed upstream or via Nix overrides, on Nix side, because it's a nix-related and temporary in nature issue. This solution, while I understand it would allow you to make progress, is quite ad-doc. I think if we start to introduce such ad-hoc solutions for every little problem now and then, the API will become a monstrosity...

inside haddock_wrapper.sh, loop over the haddock call and progressively removes failing package until it converges to a working solution. That's fragile but may work.

This is doable.

inside the haskell_doc rule, test for the presence of the package.haddock file for each sub package.

I don't think you can easily (if at all) do that from a normal rule, you can't really get output of shell commands or something like that there.

@guibou
Copy link
Contributor Author

guibou commented Jul 15, 2018

From the test in the PR, should I infer mtl is one such package?

I had issues with spatial-math for example. Have a git grep dontHaddock in a nixpkgs clone and you'll get a non-exhaustive list of packages with are known to have broken haddock. Most of the issues are due to nixpkgs packaging. I had a few coming from my client private repositories too.

IMO, if nix packages things incorrectly that should be fixed upstream or via Nix overrides, on Nix side, because it's a nix-related and temporary in nature issue. This solution, while I understand it would allow you to make progress, is quite ad-doc. I think if we start to introduce such ad-hoc solutions for every little problem now and then, the API will become a monstrosity...

I agree.

inside haddock_wrapper.sh, loop over the haddock call and progressively removes failing package until it converges to a working solution. That's fragile but may work.

This is doable.

Will you consider this for merging?

This being said, I won't champion this PR for long, I'm not stuck because of this issue (I solved my upstream problems), but I wanted to provide an easy solution for users which may encounter this issue in the future. I agree that it is an ad hoc solution for a rare problem, so we can close as won`t fix and instruct our users to fix their upstream problems.

@mboes
Copy link
Member

mboes commented Jul 15, 2018

If you don't need this, then let's close for now. The prebuilt_dependencies mechanism is a short-term hack anyways, that needs fixing (see #75).

That said, making haddock_doc more tolerant of missing haddocks in dependencies, by checking whether the .haddock file exists, sounds like a good idea to me.

@mboes
Copy link
Member

mboes commented Jul 18, 2018

That said, making haddock_doc more tolerant of missing haddocks in dependencies, by checking whether the .haddock file exists, sounds like a good idea to me.

@guibou could you file that as a ticket and close this PR?

@guibou
Copy link
Contributor Author

guibou commented Jul 18, 2018

@mboes I think that #335 is appropriate.

@guibou guibou closed this Jul 18, 2018
@guibou guibou deleted the guibou/doc_ignore_deps branch September 3, 2018 17:30
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