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

Haddock flags precedence and local override #572

Merged
merged 1 commit into from
Jan 18, 2019

Conversation

guibou
Copy link
Contributor

@guibou guibou commented Jan 15, 2019

The rational behinds is that haddocks_flags can be used to override compiler_flags. For example, if compiler_flags uses -Werror, we may want to disable it using -Wwarn in haddock_flags to allows non-fatal warnings.

Future haddock version will support -Werror in haddock.

@@ -85,6 +83,10 @@ def _haskell_doc_aspect_impl(target, ctx):
ghc_args.add([x.path for x in set.to_list(target[HaskellLibraryInfo].source_files)])
ghc_args.add(["-v0"])

# haddock flags should take precedence over ghc args, hence are in
# last position
ghc_args.add(hs.toolchain.haddock_flags)
Copy link
Contributor

@picnoir picnoir Jan 16, 2019

Choose a reason for hiding this comment

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

FYI, using args.add in order to append a list has been deprecated. This will result in an error in a future Bazel release. We should use ghc_args.add_all() instead.

See https://docs.bazel.build/versions/master/skylark/backward-compatibility.html#new-args-api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Fixed.

Copy link
Contributor

@Profpatsch Profpatsch left a comment

Choose a reason for hiding this comment

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

LGTM after switching to add_all.

@guibou guibou force-pushed the haddock_flags_precedence branch 2 times, most recently from f057a10 to d42266e Compare January 16, 2019 10:11
@guibou
Copy link
Contributor Author

guibou commented Jan 16, 2019

@Profpatsch if you can redo a review, I pushed a new commit which also add custom haddock flag per rule.

@guibou guibou changed the title Haddock flags should take precedence over compiler flags Haddock flags precedence and local override Jan 16, 2019
@mboes
Copy link
Member

mboes commented Jan 16, 2019

The latter change I object to. We have separate documentation targets. Those alone should encapsulate all documentation concerns from the moment they exist.

@guibou
Copy link
Contributor Author

guibou commented Jan 16, 2019

@mboes what do you suggest?

Use case, we use -Werror on all haddock rules, set in haskell_toolchain/haddock_rules, but one haskell_library documentation fails because of a documentation error we don't care about for now, so we want to switch to -Wwarn for that rule only. This is a real use case I have now.

@mboes
Copy link
Member

mboes commented Jan 17, 2019

If haskell_doc is composable (if it's not, then it should be made so), then you can deal with your use case like this:

haskell_doc(
  name = ":some_haskell_lib_doc",
  deps = [":some_haskell_lib"],
  haddock_flags = ["-Wwarn"],
)

haskell_doc(
  deps = [":another_lib", ":yet_another_lib", ":some_haskell_lib_doc"],
)

@guibou
Copy link
Contributor Author

guibou commented Jan 17, 2019

haskell_doc does not have haddock_flags, they are in haskell_toolchain for simplicity reason.

Should I revert this change in the hope that someone will later introduces the change you propose in haskell_doc?

@mboes
Copy link
Member

mboes commented Jan 17, 2019

Should I revert this change in the hope that someone will later introduces the change you propose in haskell_doc?

There are two commits in this PR. The first is fine and the second is controversial. Therefore, merge the first one, drop the second one and create a ticket for a composable haskell_doc. If it's relevant for your use case go ahead with the implementation of that ticket.

@guibou
Copy link
Contributor Author

guibou commented Jan 17, 2019

I just opened #583, could you please complete it with your design ideas and technical solutions.

@Profpatsch
Copy link
Contributor

Okay, let’s discuss the change there and we can merge this .

@Profpatsch Profpatsch merged commit d5633f1 into master Jan 18, 2019
@mboes mboes deleted the haddock_flags_precedence branch November 9, 2022 15:17
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.

4 participants