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

wrapFirefox renames #12299

Merged
merged 6 commits into from
Jan 15, 2016
Merged

wrapFirefox renames #12299

merged 6 commits into from
Jan 15, 2016

Conversation

vcunat
Copy link
Member

@vcunat vcunat commented Jan 10, 2016

Fixes #12065. It's a thing that seems to confuse people.

Reviews are welcome; see individual commit messages for details. /cc @NixOS/nixpkgs-committers as it's a UX issue potentially affecting many people.

After it's clearer this/which solution is taken, I'll write up a changelog entry and ensure other docs is in sync.

- I don't think that amount of code belonged into all-packages.nix.
- Now the default name of the wrapped package is identical
  with the command that runs the browser.
- Other defaults were changed according to how the wrapper is
  (almost always) used.
- `meta` is improved: mostly inherited with priority above
  the unwrapped package.
@mention-bot
Copy link

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

@abbradar
Copy link
Member

Yay! Just a week ago I had yet another question from someone regarding this.

@vcunat vcunat mentioned this pull request Jan 10, 2016
@vcunat
Copy link
Member Author

vcunat commented Jan 13, 2016

After this PR, nix-env -u will upgrade unwrapped browsers to the wrapped versions, which seems fine... but wrapped browsers will just remain silently without upgrade and installing will try to have both versions in the environment. I'm afraid we can't really improve this without changing the way nix-env -u works.

- I chose to keep `browser-unwrapped` attributes so that it's much
  easier to override parameters for the browser (through `packageOverrides`).
- Aliases `browserWrapper` are retained for now, as usual.
I'm not certain about this, so I'm trying for firefox only.
Rationale: it might be confusing to see two firefox-${version} instances
in logs or paths, so I wanted to differentiate them.
The name wasn't suggesting what kind of stuff is in there;
now it's the same as the name of the file that gets generated.
@vcunat vcunat merged commit 5fe6860 into NixOS:master Jan 15, 2016
@vcunat vcunat deleted the p/browser-wrap branch January 15, 2016 08:02
@vcunat
Copy link
Member Author

vcunat commented Jan 15, 2016

I also corrected the wrapper references in our wiki to match the current state.

@edolstra
Copy link
Member

Not sure this was an improvement. The previous names (firefox and firefox-with-plugins) seem more accurate to me. The new firefox is a misnomer: it's actually firefox plus a bunch of stuff.

@ttuegel
Copy link
Member

ttuegel commented Jan 15, 2016

The new name is much more discoverable. Before, if a new user wanted Firefox, they actually needed firefox-with-plugins in almost every case and it wasn't clear to them why. Now, if they want Firefox, the just need firefox, no explanation needed. Only an experienced Nixpkgs user would want the un-wrapped Firefox, but we may they will know how to find it in either case.

@vcunat
Copy link
Member Author

vcunat commented Jan 15, 2016

The new firefox is a misnomer: it's actually firefox plus a bunch of stuff.

By default it's only some gstreamer stuff which is probably useful for html5 playback.

@vcunat
Copy link
Member Author

vcunat commented Jan 15, 2016

BTW, I believe we still do need some general user-approachable framework for handling plugins.

@edolstra
Copy link
Member

@ttuegel No, you only install firefox-with-plugins if you want plugins, so it's perfectly discoverable.

If you actually need firefox-with-plugins for HTML5 playback, that's a bug, and we should fix that instead. (I.e., the firefox base package should depend on gstreamer, which in fact it already does.)

@vcunat I agree.

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.

7 participants