Add --haddock-output-dir flag to cabal haddock.

[LiisiKerik]

This flag gives the user full control over the directory where the documentation is placed. Closes #8270

Tested manually. Documentation for the new flag is displayed with --help. Behavior without the new flag seems the same. The new flag works both in cabal.project and from the command line and behaves as expected. No new unit tests added.

[LiisiKerik]

How do I link this PR to the issue?

[ffaf1]

How do I link this PR to the issue?

Edit your OP and add “closes #xyzx” where xyzx is the number of your ticket. Then you will see it in ”Development” in the right bar, both in this PR and in the issue itself.

[ulysses4ever]

We certainly need a test.

[LiisiKerik]

We certainly need a test.

Could you please give me some advice and direction for writing a test (where to add the test, how the feature should be tested)? This is my first time using Haskell outside uni or hobby projects. I tried looking at the test suite but found it very confusing.

[ulysses4ever]

First, it's readme at https://github.com/haskell/cabal/tree/master/cabal-testsuite (don't read past FAQ though).

Second, try to find somewhat similar test in https://github.com/haskell/cabal/tree/master/cabal-testsuite/PackageTests, copy-paste and edit it... I'd probably look into tests under *Haddock* directories. E.g. you could probably go close to https://github.com/haskell/cabal/blob/master/cabal-testsuite/PackageTests/NewHaddock/DisableDoc/cabal.test.hs but with checking output directories and files as in https://github.com/haskell/cabal/blob/master/cabal-testsuite/PackageTests/Haddock/setup.test.hs

[ulysses4ever]

@LiisiKerik do you need more directions w.r.t. tests other than what I wrote here #8788 (comment)? I hope we can bring this over the line soon.

[LiisiKerik]

@ulysses4ever Thank you, those references to other test files were very useful. I wrote a couple of tests (which at first seemed to behave reasonably) but then noticed the existence of --open flag and started to investigate how it works with the new flag. The following issue came up. When we run cabal haddock --haddock-odir=Docs --open A in a folder (let's say RunFromHere) which contains folder A the docs get placed in RunFromHere/A/Docs but the --open flag looks for docs in RunFromHere/Docs and fails. So now I'm trying to ensure that the docs get placed in the path relative to the original folder, not to the argument folder.

[LiisiKerik]

@ulysses4ever @Kleidukos @coot I added a couple of tests and the bug mentioned above is fixed. I'm not sure I picked the best place to make the --haddock-odir path absolute - it would be nice if there was a better place or way to do it.

[Kleidukos]

@ulysses4ever Personally I'm inclined to merge it and do some manual QA on it.

[LiisiKerik]

@coot @Mikolaj Is there something else I should fix in order to make it ok to merge? @Kleidukos If I understood the comment correctly then you thought it was ok to merge? If you have already looked at my code and decided that it's ok then could you please mark this PR as reviewed?

[Mikolaj]

@LiisiKerik: thank you very much the tests. This is a big PR, so we just need to throw a lot pairs of eyes at it. What is your design for backward compatibility here? Are there any corner cases in which people's workflows (e.g, in CI or scripts) stop working after this PR? Unexpected results for workflows that keep working?

[LiisiKerik]

@Mikolaj What are the possible backward compatibility issues? I probably don't understand the big picture very well since it's my first pull request in cabal.

Are there any corner cases in which people's workflows (e.g, in CI or scripts) stop working after this PR? Unexpected results for workflows that keep working?

What would be a good way to find out? I'm only aware that the automatic checks in the server all pass but what other things could I do to find the possible issues?

[Mikolaj]

Pardon me, if I'm asking silly questions. You know your code. Did you change any defaults for the other flags, for example? Is the behaviour of cabal with the default setting of the new flag (when the user does not specify it) the same as it was before this PR?

Another angle would be a few manual tests. Move your ~/.cabal aside, mkdir ~/.cabal, and run cabal update with a binary of cabal that includes your PR. Then switch to an old cabal, e.g., version 3.4 and try to build a package and some haddocks (you'd need to remove a Nix flag from ~/.cabal/config after it fails parsing it, because we neglected precisely this kind of testing for cabal 3.10).

Then test the other way around to see how new cabal works with old ~/.cabal config files.

Perhaps try the newest haddock and then a version from a year ago.

Your PR is substantial (and from what I can quickly see, it non-trivially changes and improves the codebase), hence the extra burden. I apologise that our CI does not do such tests automatically, but tests that mix versions of tools are especially tricky to automate and then to triage when they fail.

Edit: countless typos

[LiisiKerik]

@coot @ulysses4ever Is there something else I should fix in order to make it ok to merge?

[ulysses4ever]

One more thing: why the Show instances? Are they really needed for anything? Bear in mind that deriving is not free: we pay for it with the time on every compilation.

[ulysses4ever]

Personally I'm inclined to merge it and do some manual QA on it.

Just for the record: I applaud the manual QA effort but I’d be frustrated if we’re gonna use it as an excuse for not adding tests. I’m very glad that the PR author added some.

[LiisiKerik]

One more thing: why the Show instances? Are they really needed for anything? Bear in mind that deriving is not free: we pay for it with the time on every compilation.

Those were an accidental leftover from testing. I removed those now.

[ulysses4ever]

Thank you!

One last comment: the changelog file supports Markdown markup. I'd add backticks to commands like cabal haddock and the flag name. That way our release notes will look real slick.

[LiisiKerik]

Thank you!

One last comment: the changelog file supports Markdown markup. I'd add backticks to commands like cabal haddock and the flag name. That way our release notes will look real slick.

Does the synopsis field also support markdown? Or only the description?

[ulysses4ever]

@LiisiKerik both. We should document it of course...

[LiisiKerik]

Since I'm new to the process, I don't know what to do next now that the changes have been marked as approved. Do I (or someone else?) need to change tags on the pull request? Do merges happen continuously or only before releases?

[ffaf1]

Add a merge me or squash+merge tag, then wait patiently for two days.

[ulysses4ever]

There's just one commit, so the merge-me label should suffice.

[LiisiKerik]

@ffaf1 @ulysses4ever Thank you. But, it seems that I don't have the right to add labels. I don't see the gear icon next to labels.

[Kleidukos]

I'll do it, thank you for your patience @LiisiKerik :)

[LiisiKerik]

This pull request cannot be embarked for merge
The merge queue pull request can't be updated
Details:

Unable to update: user LiisiKerik is unknown.

Please make sure LiisiKerik has logged in Mergify dashboard.

Sorry for another question... but what is the mergify dashboard and how do I log in there?

[ulysses4ever]

@mergify rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[ulysses4ever]

@LiisiKerik sorry, it's just a confusing error message: i don't believe you can do anything about it. Let's see if my command helps.

[ulysses4ever]

And it's done! @LiisiKerik thanks a lot for the great contribution and patience along the way!