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

gst_all_1: make extensible #49723

Closed
wants to merge 1 commit into from

Conversation

lopsided98
Copy link
Contributor

@lopsided98 lopsided98 commented Nov 4, 2018

It was previously difficult to override a single gstreamer package and cause it to be used by other gstreamer packages. This PR converts gst_all_1 to an extensible attrset, which fixes this problem. I'm not sure that this is perfect implementation, I just based it off other examples I found in nixpkgs.

It is now possible to override a gstreamer package using something like this:

gst_all_1.extend (self: super: {
  gst-plugins-bad = (super.gst-plugins-bad.overrideAttrs (old: rec {
    buildInputs = old.buildInputs ++ [ self.rtmpdump ];
  }));
});

This PR only directly causes one rebuild, because gst-validate used to have gstreamer 0.10 passed as a dependency, which was inherited from the top level package set, rather than from gst_all_1.

  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@FRidh
Copy link
Member

FRidh commented Nov 4, 2018

Please add your example to the commit message.

@jtojnar
Copy link
Contributor

jtojnar commented Nov 4, 2018

What is the difference between makeExtensible and makeScope (used for example in gnome3 attrset)?

@lopsided98
Copy link
Contributor Author

AFAIK, newScope defines a new callPackage function that allows (in this case) any of the packages in gst_all_1 to automatically be passed to derivation functions. This allowed me to remove all the inherit gst-plugins-base; lines, and prevents errors like forgetting to pass the correct version of gstreamer.

makeExtensible adds the extend function, which allows packages in the attrset to be overridden. For example, previously you could have tried to override a package using something like this (in an overlay):

gst_all_1 = super.gst_all_1 // {
  gst-plugins-base = super.gst_all_1.gst-plugins-base.override (old: {
    enableWayland = false;
  });
};

In this case, gst_all_1.gst-plugins-base would refer to the overridden package, but gst_all_1.gst-plugins-good would still depend on the original gst-plugins-base package.

Now, the override could be done like this:

gst_all_1 = super.gst_all_1.extend (gstSelf: gstSuper: {
  gst-plugins-base = gstSuper.gst-plugins-base.override (old: {
    enableWayland = false;
  });
});

gst-plugins-good will now correctly be built with the overridden version of gst-plugins-base.

I based my implementation off of linuxPackagesFor.

@worldofpeace
Copy link
Contributor

@lopsided98

] ++ (with gst_all_1; [ gstreamer gst-plugins-base gst-plugins-bad (gst-plugins-good.override { gtkSupport = true; }) gst-libav ]);

I guess this needs adaptations to work with this? (see eval)
Possibly others as well.

It was previously difficult to override a single gstreamer package and cause it
to be used by other gstreamer packages. This commit converts gst_all_1 to an
extensible attrset, which fixes this problem.

Here is an example of how to override a gstreamer package in an overlay:

gst_all_1 = super.gst_all_1.extend (gstSelf: gstSuper: {
  gst-plugins-base = gstSuper.gst-plugins-base.override (old: {
    enableWayland = false;
  });
});
@lopsided98
Copy link
Contributor Author

That problem should be fixed now. That was caused by my attempt to use callPackages instead of callPackage, which creates its own (int this case, incorrect) override function for each package.

I really would like someone knowledgeable in this area of nixpkgs to look at this PR. There are lots of package sets that follow a similar pattern to gstreamer, and very few provide an easy way to extend them in an overlay. Some of the larger packages sets such as perl and python packages use their own adhoc methods to allow overrides. python has the packageOverrides argument, while perlPackages has the overrides argument which both have different semantics. Even Python's packageOverrides does not work well in overlays.

The more I think about it, the more I think we need a more standardized way to override/extend nested package sets.

@danbst
Copy link
Contributor

danbst commented Jan 18, 2019

Alternative, more generic solution in #54266:

$ code='with import ./. { overlays = [
    (self: super: {
        _merge_gst_all_1 = true;
        gst_all_1.gst-plugins-bad = super.gst_all_1.gst-plugins-bad.overrideAttrs (old: rec {
            buildInputs = old.buildInputs ++ [ self.rtmpdump ];
        });
    })
]; }'

$ nix-build -E "$code; arravis" # <-- will rebuild arravis with our overriden gst-plugins-bad

The _merge_gst_all_1 = true; can also be specified in Nixpkgs, so user overlays will treat gst_all_1 as a mergeable attrset.

@lopsided98
Copy link
Contributor Author

Reading through the related PRs, it seems to me that this PR shouldn't be merged without a standard extension system such as #51213.

@lopsided98 lopsided98 closed this Jan 18, 2019
@lopsided98 lopsided98 deleted the gstreamer-extensible branch October 31, 2020 18:42
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

6 participants