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

ghc8107-ghc923: patch haddock to generate correct source links #177948

Merged

Conversation

lf-
Copy link
Member

@lf- lf- commented Jun 16, 2022

Previously links to external modules were jacked because haddock was
doing them wrong. I fixed this upstream in early May 2022 but it's not
out yet.

PR link haskell/haddock#1482

This would go very nicely with #177938, as the hyperlinked sources are much more useful when the hyperlinks work.

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/) I tested ghc923 and it works great but didn't have time to test the others
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Previously links to external modules were jacked because haddock was
doing them wrong. I fixed this upstream in early May 2022 but it's not
out yet.
@cdepillabout
Copy link
Member

cdepillabout commented Jun 20, 2022

@lf- We talked a little about this in the Nix-Haskell matrix room, and there was some reluctance to apply a patch that hasn't been released upstream yet for an issue that is non-critical.

Could you explain exactly what this is fixing (along with screenshots and examples if possible)? It is hard to argue to merge this in without a simple explanation to judge the usefulness.

@lf-
Copy link
Member Author

lf- commented Jun 20, 2022

Thanks for the feedback.

Here is an example of the issue it fixes (package picked because it has few deps):

$ nix-build -A haskellPackages.hscolour.doc
/nix/store/2h9kw29hmaw6qfksl5y434wmhxiz4x8d-hscolour-1.24.4-doc

$ firefox result-doc/share/doc/hscolour-1.24.4/html/src/Language.Haskell.HsColour.General.html

Then click on "Eq" in one of the constraints. The link will not resolve correctly without the patch, because haddock was generating urls like ../file:///nix/store/gjrx6ps8xpqf983qcp69smdqal9ydk81-ghc-9.0.2-doc/share/doc/ghc/html/libraries/ghc-prim-0.7.0/src, which are missing any file name for the appropriate module, and also begin with ../. This makes haddock hyperlinked source browsing very annoying, especially in libraries further up the stack that integrate heavily with other libraries.

I fixed the URL generation bug above with this patch. There are still other remaining hyperlinking bugs in Haddock, for instance haskell/haddock#1498, but I think the bug this patch fixes is one of the bugs most hampering the usefulness of the hyperlinked sources feature (besides base not having hyperlinked sources, which is now fixed).

It's totally fine if it's not worth patching nixpkgs to fix this, but it significantly hampers the usefulness of the source hyperlinking feature that very many of the links are broken.

@cdepillabout
Copy link
Member

cdepillabout commented Jun 28, 2022

I've taken a look at the current source docs, and I've confirmed that it is quite annoying to work with the hyperlinked sources, since the links don't work. This PR does appear to fix things and make the hyperlinked sources quite useful.


@lf- We're somewhat reluctant to merge this PR, since we don't want to get into the habit of carrying around a large number of patches on top of GHC here in Nixpkgs, especially for non-critical functionality like this that hasn't been released yet upstream.

However, this does seem like it would be a nice fix, and it is arguably fixing a current bug.

We'd like to merge this PR, but on the condition that you watch the haskell-updates branch for any breakage this PR might cause. Would you be willing to do that, at least for the next week or two? You'd generally have to watch the following things, and look for any failures due to the additional patch in this PR (although I imagine it is not likely it causes any problems):

@lf-
Copy link
Member Author

lf- commented Jun 28, 2022

Sure, I'll check it in the morning when I get into work for the next week or so. For what it's worth, we applied this in a large Yesod codebase with lots of deps at my employer about a week ago and there was no build breakage.

@cdepillabout cdepillabout merged commit c9e0650 into NixOS:haskell-updates Jun 28, 2022
@cdepillabout
Copy link
Member

Thanks.

Here's the evaluation with the change from this PR: https://hydra.nixos.org/eval/1769156

Here's the parent evaluation that doesn't contain the change from this PR: https://hydra.nixos.org/eval/1769150

You should hopefully be able to compare these for any differences. These have both been taken from https://hydra.nixos.org/jobset/nixpkgs/haskell-updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants