Navigation Menu

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

weechat: install all outputs into the final store path #59487

Merged
merged 1 commit into from May 24, 2019
Merged

weechat: install all outputs into the final store path #59487

merged 1 commit into from May 24, 2019

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Apr 14, 2019 •
edited

Motivation for this change

Resolves #59300

Until now only $out/bin/weechat and $out/bin/weechat-headless were installed
into the store path that will be used when running i.e. nix-env -iA weechat.

Further outputs like icons ($out/share/icons), man pages ($man) or the HTML
documentation ($out/share/doc/weechat) are omitted at the moment. As
this can be fairly confusing I figured that it's better to copy those
files into the environment as well.

As buildEnv doesn't appear to support output splitting (you can only
install additional outputs of paths using extraOutputsToInstall),
it's easier for now to always install the man output by
default.

Man page installation can be turned off like this now:

weechat.override { installManPages = false; }
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Ma27
weechat: install all outputs into the final store path
681b1d8 
Resolves NixOS#59300

Until now only `$out/bin/weechat` and `$out/bin/weechat-headless` were installed
into the store path that will be used when running i.e. `nix-env -iA weechat`.

Further outputs like icons (`$out/share/icons`), man pages (`$man`) or the HTML
documentation (`$out/share/doc/weechat`) are omitted at the moment. As
this can be fairly confusing I figured that it's better to copy those
files into the environment as well.

As `buildEnv` doesn't appear to support output splitting (you can only
install additional outputs of `paths` using `extraOutputsToInstall`),
it's easier for now to always install the `man` output by
default.

Man page installation can be turned off like this now:

```
weechat.override { installManPages = false; }
```
@teto
Copy link
Member

teto commented Apr 14, 2019

I've seen this problem with zathura and neovim too, seems like we need buildEnv to support output splitting ? (in another PR maybe).

@Ma27
Copy link
Member Author

Ma27 commented Apr 14, 2019

Hmm indeed, in the long term we should fix this for buildEnv. I guess that this patch is mergable as-is, I'll have a look at the build script of buildEnv in the next weeks and see how to implement this :)

@Ma27 Ma27 mentioned this pull request Apr 18, 2019
Closed
@FRidh
Copy link
Member

FRidh commented Apr 18, 2019 •
edited

Perhaps

passthru.man = weechat.man;

That way it is still a separate output. Not sure if buildEnv would be smart enough to pick this man up as an output though.

@Ma27
Copy link
Member Author

Ma27 commented Apr 18, 2019

Not sure if buildEnv would be smart enough to pick this man up as an output though.

If I understood how buildEnv works this won't work correctly:

  • extraOutputsToInstall installs all given outputs into the environment by default (that's what this patch does)
  • With passthru being set, man won't be detected as additional output. Hence users have to install man pages for weechat manually.

I think that for now it's fine to use extraOutputsToInstall here. In the long term we probably want to fix buildEnv to support extra outputs.

@Ma27
Copy link
Member Author

Ma27 commented May 23, 2019

@FRidh anythign else? I don't have the time to fix buildEnv accordingly at the moment, so I think that this is the best way to go for now.

@FRidh
Copy link
Member

FRidh commented May 23, 2019

Nope

@Ma27
Copy link
Member Author

Ma27 commented May 24, 2019

Great! :)

@Ma27 Ma27 merged commit 8226346 into NixOS:master May 24, 2019
@Ma27 Ma27 deleted the install-weechat-manpages branch May 24, 2019 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No manpage for weechat
4 participants
@Ma27 @teto @FRidh @GrahamcOfBorg