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

Don't output haddock stdout if verbosity is silent #7483

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Jul 13, 2021

Extracted from #7478

Basically, if verbosity is silent, don't output haddock to stdout.

As a non-ideal sideeffect, stderr output is swallowed if haddock has a zero-exit code.


Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

@Mikolaj
Copy link
Member

Mikolaj commented Jul 13, 2021

As a non-ideal sideeffect, stderr output is swallowed if haddock has a zero-exit code.

I think this is perfectly acceptable, but a comment to that effect by the relevant code would be nice.

@fendor
Copy link
Collaborator Author

fendor commented Jul 13, 2021

Documenting in the code sounds to me like not documenting it at all. Not many people read the Cabal code-base.

Documentation is necessary, but I am unsure where. Documenting it in the code seems to me not ideal, as most people won't read the code (it took me 3 years to reach that code-path). Maybe we should document in the user-guide/reference?

Edited such that the comment is not repellent any more

@Mikolaj
Copy link
Member

Mikolaj commented Jul 13, 2021

OK, I will shut up and let you decide how to document it.

@fendor
Copy link
Collaborator Author

fendor commented Jul 13, 2021

@Mikolaj My apologies, I did not mean it like that! I meant, I was thinking on how to document it properly and did not mean to disregard your comment! It needs to be documented, I am just not sure how.

@Mikolaj
Copy link
Member

Mikolaj commented Jul 13, 2021

No problem at all. Is that really a user-facing feature? Isn't text on stderr with no error exit code a pathology? If so, I wouldn't worry. But I may be missing something, e.g., haddock calling some tool, which exits printing error message that the user should see even though haddock carries on?

@fendor
Copy link
Collaborator Author

fendor commented Jul 13, 2021

But I may be missing something, e.g., haddock calling some tool, which exits printing error message that the user should see even though haddock carries on?

Oh I have no idea. But it is sure to break someones workflow

https://xkcd.com/1172/

@Mikolaj
Copy link
Member

Mikolaj commented Jul 13, 2021

Right. Given that, a changelog entry wouldn't hurt, just in case.

@Mikolaj
Copy link
Member

Mikolaj commented Jul 13, 2021

Actually, I've just tested and haddock prints (some) warnings to stderr, e.g., "Warning: The documentation for the following packages are not installed.".

@Mikolaj
Copy link
Member

Mikolaj commented Jul 13, 2021

All other warnings seem to be printed to stdout. That's probably wrong in itself, but perhaps also may influence this PR.

@fendor
Copy link
Collaborator Author

fendor commented Jul 13, 2021

I don't mind polluting stdout in #7478 as we have the file output option. Maybe we can raise some bug reports in Haddock itself.

@fendor fendor force-pushed the enhance/silence-haddock-output branch 3 times, most recently from 5cfdb14 to 447c5e7 Compare July 14, 2021 09:51
@fendor
Copy link
Collaborator Author

fendor commented Jul 14, 2021

incredible, now I am forced to boot into windows to understand why this fails on windows >_>

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 1, 2022
@ulysses4ever ulysses4ever removed the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 3, 2022
@ulysses4ever
Copy link
Collaborator

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Sep 22, 2022

rebase

❌ Base branch update has failed

Git reported the following error:

Rebasing (1/1)
error: could not apply 0a1dec884... Don't output haddock stdout if verbosity is silent
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 0a1dec884... Don't output haddock stdout if verbosity is silent
Auto-merging cabal-testsuite/src/Test/Cabal/OutputNormalizer.hs
CONFLICT (content): Merge conflict in cabal-testsuite/src/Test/Cabal/OutputNormalizer.hs
Auto-merging cabal-testsuite/src/Test/Cabal/Monad.hs
CONFLICT (content): Merge conflict in cabal-testsuite/src/Test/Cabal/Monad.hs
Auto-merging cabal-testsuite/PackageTests/NewHaddock/Fails/cabal.out
CONFLICT (content): Merge conflict in cabal-testsuite/PackageTests/NewHaddock/Fails/cabal.out
Auto-merging Cabal/src/Distribution/Simple/Haddock.hs

err-code: DE066

@ulysses4ever
Copy link
Collaborator

It's a pity this has been languished. @fendor do you by any chance have energy to bring it over the line? There's one approval already, so that makes it really close. I think minor annoyances around stderr and Haddock failures are not as important.

@fendor
Copy link
Collaborator Author

fendor commented Sep 22, 2022

@ulysses4ever Unfortunately, not likely at the moment. I have no use-case for it and if I have time to work on cabal related PRs, it will be #7500, since I expect it to help cabal and HLS to operate much better together, which I do have a use-case for.

@Kleidukos
Copy link
Member

Marking this PR as draft 🙂

@Kleidukos Kleidukos marked this pull request as draft May 17, 2023 06:37
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.

5 participants