-
Notifications
You must be signed in to change notification settings - Fork 711
Fix haddock via Setup.hs with internal libraries #7827
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
Conversation
I wrote a test that failed and then made a fix that made it pass. I'm not sure if this is the correct fix, though. Would love guidance on an alternate fix |
Hi @brandonchinn178! Thank you for the test that shows there is a problem and for the fix. Does anybody have any comments? The CI failure seems to be not a random breakage, but related to haddocks, so probably there is some more work to do. |
Sorry forgot to commit the output for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Let's hope this fixes the stack use case.
@Mikolaj Thanks! Is there anything else I need to do to get this merged? |
@brandonchinn178: lure one more reviewer with cookies. |
@brandonchinn178 I tried your branch and I still get the error, but I'm not sure if I did the right things to actually use the dev Regardless, I'm not really able to review your code itself, but once I can verify it fixes my problem I'd be happy to give a thumbs-up on that basis. |
@hsenag: I'm afraid you might have used Cabal library from hackage. That's why README says to run
and run the cabal command above in there. |
@hsenag: ping? |
@Mikolaj sorry for the delay, I've kicked off going through the steps now. Feel free to ping me again in a day or two if I don't report back. |
I'm still seeing the failure. What I've done this time:
and I still get
|
Oh. So we have a repro. Thank you @hsenag. @brandonchinn178: are you able to reproduce the failure with darcs-2.17.1? Would you like me to try? |
I use Stack primarily, so I'm not sure how much help I'll be in debugging Cabal invocations. If you could take a look, that'd be great. Thanks! |
FYI I've now verified that this also happens with |
@hsenag: I confirm your results, both with and without the However, I see @brandonchinn178: did you intend to fix this for Here's the -v2 log of the run without
|
The function I edited should be the single entrypoint for everything. Short of a custom Setup.hs script that overrides the haddock hook with some completely adhoc function, my fix should still apply. I'll try to download the darcs code today and take a look at the Setup.hs script |
I'm afraid it does override a whole lot:
|
It might just be on us to update the hook then, if so apologies for the noise. We have been trying to get rid of our custom setup but there are still a few pieces left. |
Based on my tests on the darcs repo, the problem still happens with your changes. But hopefully it should be possible to update our hook to take account of your changes? I didn't have a chance to take a deeper look to figure that out yet, though it's on my TODO list. If you can point me in the right direction it would probably speed that up :-) |
@brandonchinn178, @hsenag: how about, to avoid mutual blocking, we merge this PR and test, fix, amend, etc., as needed, later on? |
I'd be very happy with that, and I never intended to block this PR. If I understood the start of the discussion about whether it fixed the darcs case, it was because @brandonchinn178 needed a second reviewer and that I could review from the point of view of a user whose problem was fixed. Without that, and without knowing the |
That would be very much appreciated indeed. :) |
Turns out it doesn't help because I'm not a reviewer with write access :-( |
That's fine --- I can force-merge regardless. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@hsenag, @Kleidukos: thank you so much. @brandonchinn178: once you rebase and fix the conflicts, let's merge! |
Done! |
Fixes #1919