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

mpv: Move all wrappings to a single wrapper Nix function #88620

Merged
merged 1 commit into from
May 25, 2020

Conversation

doronbehar
Copy link
Contributor

@doronbehar doronbehar commented May 22, 2020

Motivation for this change

In #88136 I concluded that mpv's wrapping was done purely - both the "unwrapped" build had done some wrappings to the two executables in $out/bin/, while we already had an external wrapper derivation called mpv-with-scripts. This was bad because:

  1. Users of mpv-with-scripts had their executable wrapped twice.
  2. disabling youtubeSupport caused mpv to be wrapped twice while it was a mere change of including youtube-dl in the wrapper's $PATH or not.
  3. If you'd want youtubeSupport enabled, but say you had an override for an updated youtube-dl, it was impossible to make mpv use your youtube-dl without recompiling it.
Things done

This PR is a rewrite of what was called mpv-with-scripts - it's now called wrapMpv (inspired by Neovim's wrapNeovim Nix function) and it's possible with it to satisfy both "deterministics" which want everything to be configured exactly as they want via Nix while it enables everyone to tweak what's possible via an external wrapper which doesn't trigger a rebuild of mpv.

I tested wrapMpv with:

  • mpv-unwrapped was compiled with and without vapoursynthSupport enabled - checked that the wrapper got the same environment but:
    • I haven't tested functionality of this feature.
  • A certain script - checked that the necessary flags are added to the wrapper and also tested that the script works.
  • Tested that youtubeSupport works (of course it works as the exectuable has the same environment).

Besides that, as always:

  • 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.

cc @pstch @AndersonTorres @jtojnar

pkgs/top-level/aliases.nix Outdated Show resolved Hide resolved
@doronbehar
Copy link
Contributor Author

@GrahamcOfBorg build mpv-unwrapped

@GrahamcOfBorg build mpv

@doronbehar
Copy link
Contributor Author

Is it me or do these build commands don't work? :/

@ajs124
Copy link
Member

ajs124 commented May 22, 2020

They only work if you're a whitelisted user.

@doronbehar
Copy link
Contributor Author

@AndersonTorres AndersonTorres added 8.has: package (update) This PR updates a package to a newer version and removed 8.has: package (new) This PR adds a new package labels May 23, 2020
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label May 23, 2020
@ofborg ofborg bot requested a review from AndersonTorres May 23, 2020 15:49
Inspired by `wrapNeovim`, write a wrapMpv Nix function that creates a
derivation that has all of the environment that was added if needed at
the unwrapped version.

Add derivations to all-packages.nix in an almost compatible way and make
`mpv-with-scripts` throw a message implying to switch to `wrapMpv` which
has an incompatible signature.

Add to vapoursynth a new passthru attribute `python3` that is used in
passed down to the wrapper to ensure ABI compatibility with
`PYTHONPATH`.
Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

Just looking from above, feels good to me, and way more organized.
Pushing out the wrap logic is a great improvement.

Copy link
Member

@teto teto left a comment

Choose a reason for hiding this comment

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

It's more inline with current nixpkgs' approach.

@AndersonTorres AndersonTorres merged commit c51d8d6 into NixOS:master May 25, 2020
@Profpatsch
Copy link
Member

It would be great if symbols like mpv-with-wrappers would get a deprecation period instead of breaking downstream immediately.

Since this is a breaking change, this requires an entry in the release notes cc @doronbehar @AndersonTorres

@doronbehar
Copy link
Contributor Author

You are correct. It was on my mind, I wanted to get some feedback before adding to the PR the changes to rl-2009 but I forgot about it after a while. See: #89208

berbiche added a commit to berbiche/home-manager that referenced this pull request May 31, 2020
berbiche added a commit to berbiche/home-manager that referenced this pull request May 31, 2020
rycee pushed a commit to nix-community/home-manager that referenced this pull request May 31, 2020
The latter has been removed from Nixpkgs.

See:

- <NixOS/nixpkgs#88620>
- <NixOS/nixpkgs#89208>

PR #1295
colemickens pushed a commit to colemickens/home-manager that referenced this pull request Jun 24, 2020
colemickens pushed a commit to colemickens/home-manager that referenced this pull request Jun 27, 2020
colemickens pushed a commit to colemickens/home-manager that referenced this pull request Jul 20, 2020
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.

5 participants