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

unbreak cabal haddock --disable-documentation #8330

Merged
merged 1 commit into from
Aug 21, 2022
Merged

Conversation

ulysses4ever
Copy link
Collaborator

@ulysses4ever ulysses4ever commented Jul 29, 2022

Trying to fix my fault from #8259 (thanks @andreasabel for noticing): cabal haddock --disable-documentation seems to work as expected with this patch.

Below is a description of a problem I initially had but no more. Preserved for historical interest.


But the testsuite is not cooperating. I see two options: you, guys, help me or allow me to ditch the test.

Here's the deal with the test:

main = cabalTest . withRepo "repo" . fails $
    cabal "haddock" ["--disable-documentation", "B"]

should work, but it fails even when

❯ cabal run cabal-tests -- --with-cabal=(cabal list-bin cabal) cabal-testsuite/PackageTests/NewHaddock/DisableDoc/cabal.test.hs --accept

I see the output I'm expecting to see, in particular:

 100% (  2 /  2) in 'B'
Warning: B: could not find link destinations for:

        - A.a

but the output ends with

cabal-tests: callProcess: /nix/store/glag1h91br1sqk489rzzp6mvmxf0pmiy-ghc-9.0.2/bin/runghc-9.0.2 "--" "--ghc-arg=-i" "--ghc-arg=-no-user-package-db" "--ghc-arg=-package-db" "--ghc-arg=/home/artem/.cabal/store/ghc-9.0.2/package.db" "--ghc-arg=-package-db" "--ghc-arg=/home/artem/tmp/cabal/enable-doc-regress/cabal/dist-newstyle/packagedb/ghc-9.0.2" "--ghc-arg=-package-id" "--ghc-arg=Cabal-3.9.0.0-inplace" "--ghc-arg=-package-id" "--ghc-arg=Cabal-syntax-3.9.0.0-inplace" "--ghc-arg=-package-id" "--ghc-arg=async-2.2.4-2b04cd6dcc0fc474e0072aec3462b9c67410113d7befe87d8f668b6a29dfe996" "--ghc-arg=-package-id" "--ghc-arg=base-4.15.1.0" "--ghc-arg=-package-id" "--ghc-arg=cabal-testsuite-3-inplace" "--ghc-arg=-package-id" "--ghc-arg=clock-0.8.3-12cb68ccaf6e6206378be8082c1ee7435ae9bbce4022f1012bd6cd3e5c8fc2aa" "--ghc-arg=-package-id" "--ghc-arg=exceptions-0.10.4" "--ghc-arg=-package-id" "--ghc-arg=filepath-1.4.2.1" "--ghc-arg=-package-id" "--ghc-arg=optparse-applicative-0.16.1.0-72520bdb9dc24ef310719d7357cf6ce0dac24c8b958b2d2663b3d141a87f2fd6" "--ghc-arg=-package-id" "--ghc-arg=process-1.6.14.0-42e0058a6e7535eea211d226cecb5365b21bf90abcca43d0f0527ff0319ccdca" "--ghc-arg=-package-id" "--ghc-arg=transformers-0.5.6.2" "cabal-testsuite/PackageTests/NewHaddock/DisableDoc/cabal.test.hs" "--builddir" "/home/artem/tmp/cabal/enable-doc-regress/cabal/dist-newstyle/build/x86_64-linux/ghc-9.0.2/cabal-testsuite-3" "cabal-testsuite/PackageTests/NewHaddock/DisableDoc/cabal.test.hs" "--with-cabal" "/home/artem/tmp/cabal/enable-doc-regress/cabal/dist-newstyle/build/x86_64-linux/ghc-9.0.2/cabal-install-3.9.0.0/x/cabal/build/cabal/cabal" "--accept" (exit 1): failed

Which is no good, as far as I understand. If I remove fails in the .hs file, the failure in the end seems to be the same. So, I'm probably using fails wrong?..


Please include the following checklist in your PR:

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

@ulysses4ever ulysses4ever added priority: high 🔥 regression on master Regression that is unreleased and needs to be fixed before release attention: needs-help Help wanted with this issue/PR labels Jul 29, 2022
@ulysses4ever
Copy link
Collaborator Author

More or less, what I see locally reproduces in the CI: https://github.com/haskell/cabal/runs/7571243588?check_suite_focus=true

@Mikolaj Mikolaj added the attention: needs-backport in the future e.g., to a point release after the main release label Jul 29, 2022
@Mikolaj
Copy link
Member

Mikolaj commented Jul 29, 2022

Is Warning: B: could not find link destinations for a hard error that exits with 1, or is it just a warning after which you continue?

@ulysses4ever ulysses4ever force-pushed the unbreak-disable-doc branch from 409301f to 501a574 Compare July 29, 2022 19:12
@ulysses4ever
Copy link
Collaborator Author

@Mikolaj wow, I was positive that removing fails didn't change the output and was still saying "fail" but now it's not. SO, removing fails works and if I --accept output and remove the patch, then the test fails, so it's good. Waiting for CI.

@ulysses4ever ulysses4ever removed priority: high 🔥 attention: needs-help Help wanted with this issue/PR labels Jul 29, 2022
@ulysses4ever ulysses4ever force-pushed the unbreak-disable-doc branch from 501a574 to 568268a Compare July 29, 2022 19:17
@ulysses4ever ulysses4ever marked this pull request as ready for review July 29, 2022 20:19
Comment on lines +2 to +3
-- Test that `cabal haddock --disable-documention` works as expected and leads
-- to a warning if a local package makes an outer reference.
Copy link
Member

Choose a reason for hiding this comment

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

Could we perhaps extend the test to also display the warning (for some other module?)? The very text of the warning may be fragile, but if we notice we need to update it too often as haddock gets message overhauls, we can grep for a couple of keywords instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. See the assert below. Is it good?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I see you approved, so I guess it's good.

@ulysses4ever ulysses4ever force-pushed the unbreak-disable-doc branch from 568268a to 97cb72e Compare July 29, 2022 23:37
@ulysses4ever
Copy link
Collaborator Author

@Mikolaj does it need a changelog entry?

@Mikolaj
Copy link
Member

Mikolaj commented Jul 30, 2022

@Mikolaj does it need a changelog entry?

I don't think so. You are fixing a change that wasn't yet released, so nobody's going to know. :)

@ulysses4ever
Copy link
Collaborator Author

Right, I forgot #8259 wasn't backported to 3.8.

@Mikolaj Mikolaj requested a review from andreasabel August 4, 2022 19:51
Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Thanks @ulysses4ever !

@ulysses4ever ulysses4ever added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-backport in the future e.g., to a point release after the main release merge me Tell Mergify Bot to merge regression on master Regression that is unreleased and needs to be fixed before release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants