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

haskell/with-packages-wrapper.nix: install "doc" outputs #76842

Merged
merged 2 commits into from Jan 7, 2020

Conversation

@matthewbauer
Copy link
Member

@matthewbauer matthewbauer commented Jan 2, 2020

We were previously just installing the "out" output which broke when
we recently changed to generating multiple outputs.

Fixes #76837

Motivation for this change
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.
Notify maintainers

cc @

We were previously just installing the "out" output which broke when
we recently changed to generating multiple outputs.

Fixes #76837
@GuillaumeDesforges

This comment has been minimized.

Does this line copy the doc directory to the ghc-with-packages nix store entry ?
Then, would it be accessible like this ?

"${ghc-with-packages}/share/doc/hoogle/default.hoo"

This comment has been minimized.

Copy link
Owner Author

@matthewbauer matthewbauer replied Jan 3, 2020

This copies the doc output, which may or may not be where the doc directory lives. The above line should work without this change since we are already generating hoogle below.

This change makes things like "${ghc-with-packages}/share/doc/ghc/html" work because the GHC derivation includes them in the doc output.

@cdepillabout
Copy link
Member

@cdepillabout cdepillabout commented Jan 4, 2020

@matthewbauer Could you show me how to test that this actually fixes the problem with hoogle?

My idea was to run a command like the following on the current master:

$ nix-build -E 'with import ./. {}; haskellPackages.ghcWithPackages (p: [p.conduit])'
/nix/store/48grjcgskic4iij38sgdfx4f4q7r0hlr-ghc-8.6.5-with-packages

This works, as expected. In the /nix/store/48grjcgskic4iij38sgdfx4f4q7r0hlr-ghc-8.6.5-with-packages directory, there is no share/doc directory (as expected?).

I then checked out this PR and tried to run the same command, but I got an error:

$ hub checkout https://github.com/NixOS/nixpkgs/pull/76842
$ nix-build -E 'with import ./. {}; haskellPackages.ghcWithPackages (p: [p.conduit])'
these derivations will be built:
  /nix/store/chk26hff7nhhzjyjj5fk80xm3rw3v597-ghc-8.6.3-with-packages.drv
building '/nix/store/chk26hff7nhhzjyjj5fk80xm3rw3v597-ghc-8.6.3-with-packages.drv'...
created 131 symlinks in user environment
rm: cannot remove '/nix/store/0s4vjgsz437aqmpyxxr5zqdcdcdih57j-ghc-8.6.3-with-packages/bin/ghc': Permission denied
builder for '/nix/store/chk26hff7nhhzjyjj5fk80xm3rw3v597-ghc-8.6.3-with-packages.drv' failed with exit code 1
error: build of '/nix/store/chk26hff7nhhzjyjj5fk80xm3rw3v597-ghc-8.6.3-with-packages.drv' failed

I don't understand two things:

  1. Why is this trying to use ghc-8.6.3 (and not the latest version of ghc, 8.6.5)? Is this PR branched off of an old version of nixpkgs? Could you rebase it on top of the current master?
  2. It is probably bad that this is failing, right?
@matthewbauer
Copy link
Member Author

@matthewbauer matthewbauer commented Jan 6, 2020

  1. Why is this trying to use ghc-8.6.3 (and not the latest version of ghc, 8.6.5)? Is this PR branched off of an old version of nixpkgs? Could you rebase it on top of the current master?

Yeah this was based off of git merge-base origin/release-19.03 origin/release-19.09 so that it can be merged into master, release-19.09 and release-19.03. It definitely can be rebased, but ideally this fix would be merged into all three branches.

  1. It is probably bad that this is failing, right?

Yeah this is definitely an oversight. It looks like it is a result in moving from symlinkJoin to buildEnv.

The wrapper need a writable directory to work, so remove the symlink
to a read-only one if it occurs.
@cdepillabout
Copy link
Member

@cdepillabout cdepillabout commented Jan 7, 2020

@matthewbauer

Yeah this was based off of git merge-base origin/release-19.03 origin/release-19.09 so that it can be merged into master, release-19.09 and release-19.03.

Oh, I see. That's really smart! I'm happy with this as-is, then. No need to rebase.

It looks like it is a result in moving from symlinkJoin to buildEnv.

I've tested your fix in 93aabab and this appears to be working for me.

$ nix-build -E 'with import ./. {}; haskellPackages.ghcWithPackages (p: with p; [aeson conduit lens servant-server])'
...
/nix/store/c36g05zmrflibzy5w6wji2zc0nmry3sf-ghc-8.6.3-with-packages
$ ls /nix/store/c36g05zmrflibzy5w6wji2zc0nmry3sf-ghc-8.6.3-with-packages/share/doc/
adjunctions-4.4
aeson-1.4.2.0
ansi-terminal-0.8.2
...
@matthewbauer
Copy link
Member Author

@matthewbauer matthewbauer commented Jan 7, 2020

Also it's usually worth trying the GitHub merge commit to test PRs. I think nix-review does this for you, but you can also use something like & test merge_commit_sha directly:

nix-build -E "(import (builtins.fetchTarball \"https://github.com/NixOS/nixpkgs/archive/$(curl https://api.github.com/repos/NixOS/nixpkgs/pulls/76842 | jq -r .merge_commit_sha).tar.gz\") {}).haskellPackages.ghcWithPackages (p: [p.conduit])"
@matthewbauer matthewbauer merged commit ddcaa0c into NixOS:master Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.