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

Jump to bottom

haskell: Add hoogle option to developPackage #103061

Merged
merged 3 commits into from Nov 13, 2020
Merged

haskell: Add hoogle option to developPackage #103061

merged 3 commits into from Nov 13, 2020

Conversation

expipiplus1
Copy link
Contributor

Also add composeExtensionss to lib.

This seems to be a sensible way of getting ghcWithHoogle into the mix.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@expipiplus1
Copy link
Contributor Author

name hoogle -> withHoogle for consistency with other makeShell

cdepillabout
cdepillabout requested changes Nov 10, 2020
View reviewed changes
Copy link
Member

@cdepillabout cdepillabout left a comment •
edited

Choose a reason for hiding this comment

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

I left a few comments.

Also, it would be nice if you wrote some documentation for end users who want to use Hoogle. Could you add a sentence or two for what an end user would be able to do by enabling the withHoogle option? I'm guessing the idea is that they now get a hoogle in their path with a database of all dependencies?

It'd also be nice to have a test for developPackage (similar to the test for shellFor), but that's more of a wish-list type thing. Not having a test shouldn't block this from being merged in.

lib/fixed-points.nix Outdated Show resolved Hide resolved
lib/tests/misc.nix Outdated Show resolved Hide resolved
pkgs/development/haskell-modules/make-package-set.nix Outdated Show resolved Hide resolved
pkgs/development/haskell-modules/make-package-set.nix Outdated Show resolved Hide resolved
pkgs/development/haskell-modules/make-package-set.nix Outdated Show resolved Hide resolved
@expipiplus1
Copy link
Contributor Author

It'd also be nice to have a test for developPackage (similar to the test for shellFor), but that's more of a wish-list type thing. Not having a test shouldn't block this from being merged in.

Such a test would involve import-from-derivation, is that permissible in nixpkgs?

@cdepillabout
Copy link
Member

Such a test would involve import-from-derivation, is that permissible in nixpkgs?

Ah, good call.

I guess you could always just mark it __noChroot and we could use it as a test that we run manually, but it would be unfortunate that we couldn't have it run by Hydra.

@expipiplus1
Copy link
Contributor Author

Such a test would involve import-from-derivation, is that permissible in nixpkgs?

Ah, good call.

I guess you could always just mark it __noChroot and we could use it as a test that we run manually, but it would be unfortunate that we couldn't have it run by Hydra.

Right, perhaps best for another PR

cdepillabout
cdepillabout approved these changes Nov 11, 2020
View reviewed changes
Copy link
Member

@cdepillabout cdepillabout left a comment

Choose a reason for hiding this comment

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

Aside from the one remaining comment, I think this looks really good.

I'm going to leave this open for a few days, so that any else that got notified has a chance to comment on the change in lib/.

@expipiplus1 If there are no additional comments to this in a few days, please feel free to ping me and I will merge it in (assuming the test thing has been fixed).

lib/tests/misc.nix Outdated Show resolved Hide resolved
@expipiplus1
Copy link
Contributor Author

Will do, thanks

@expipiplus1
Copy link
Contributor Author

@cdepillabout ping!

@expipiplus1
Copy link
Contributor Author

I don't think that @ofborg's failure is to do with this PR

@cdepillabout cdepillabout merged commit 57c9485 into NixOS:haskell-updates Nov 13, 2020
@expipiplus1
Copy link
Contributor Author

Thanks!

@expipiplus1 expipiplus1 deleted the joe-hoogle branch November 13, 2020 04:40
@FRidh FRidh mentioned this pull request Dec 19, 2020
Closed
@FRidh
Copy link
Member

FRidh commented Dec 19, 2020

composeManyExtensions was proposed in #33258.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdepillabout cdepillabout cdepillabout approved these changes

@edolstra edolstra Awaiting requested review from edolstra

@infinisil infinisil Awaiting requested review from infinisil infinisil is a code owner

@nbp nbp Awaiting requested review from nbp

Assignees
No one assigned
Labels
6.topic: haskell 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@expipiplus1 @cdepillabout @FRidh