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

closure-size: nix-serve: include bzip2 on PATH #12771

Closed
wants to merge 1 commit into from

Conversation

ttuegel
Copy link
Member

@ttuegel ttuegel commented Feb 2, 2016

This fixes nix-serve on the closure-size branch.

/cc @vcunat

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @edolstra and @wkennington to be potential reviewers

@dezgeg
Copy link
Contributor

dezgeg commented Feb 2, 2016

Actually bzip2 should be coming from here: https://github.com/NixOS/nixpkgs/blob/closure-size/nixos/modules/services/networking/nix-serve.nix#L53. Which means we should do the output selection thing in systemd.services.<foo>.path as well.

@ttuegel
Copy link
Member Author

ttuegel commented Feb 2, 2016

@dezgeg Thanks! We'll do that in another PR. Closing this one.

@ttuegel ttuegel closed this Feb 2, 2016
@ttuegel ttuegel deleted the closure-size-nix-serve branch February 2, 2016 15:26
@edolstra
Copy link
Member

edolstra commented Feb 2, 2016

Well, arguably it's cleaner to fix it in the wrapper script than in the systemd service, since the latter makes it work for all users.

@ttuegel
Copy link
Member Author

ttuegel commented Feb 2, 2016

@edolstra Well, the wrapper script was missing bzip2, so if you weren't using the systemd service, it was already broken. But @dezgeg pointed out (rightly) that all systemd services are having their PATH set improperly; we should be doing map (pkg: pkg.bin or pkg) pkgs for the packages listed there.

@edolstra
Copy link
Member

edolstra commented Feb 2, 2016

That seems hacky. Why pkg.bin or pkg? Why not pkg.bin or pkg.bin or ... or pkg? Rather, it should use whatever outputs would be installed into a user environment by default.

@ttuegel
Copy link
Member Author

ttuegel commented Feb 2, 2016

Rather, it should use whatever outputs would be installed into a user environment by default.

Yes, absolutely. That's what I meant. I may not have quoted the expression correctly. 😃

@dezgeg
Copy link
Contributor

dezgeg commented Feb 2, 2016

And for clarification: this is what the closure-size branch currently uses for choosing what to install in the user environment: pkg.bin or pkg.out or pkg.
#12653 for the discussion on that.

@ttuegel
Copy link
Member Author

ttuegel commented Feb 8, 2016

And for clarification: this is what the closure-size branch currently uses for choosing what to install in the user environment: pkg.bin or pkg.out or pkg.

No, because that PR was never merged.

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.

None yet

4 participants