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

kodi: add Wayland support #58400

Merged
merged 2 commits into from Apr 15, 2019
Merged

kodi: add Wayland support #58400

merged 2 commits into from Apr 15, 2019

Conversation

minijackson
Copy link
Member

@minijackson minijackson commented Mar 26, 2019 •
edited

Motivation for this change

Kodi has had support (again) for Wayland since v18.0. It would be nice to ship it in NixOS.

I mimicked Firefox for Wayland's attribute path for Kodi with plugins:

  • New "plain" Kodi: kodiPlainWayland
  • kodiPlainWayland gets wrapped with plugins into kodi-wayland

I'm open to suggestions / discussion, particularly on 2 things:

  • The libxkbcommon build dependency is specified as libxkbcommon.dev, otherwise kodiPlainWayland won't build for me (cmake doesn't find xcbcommon), and I don't know why :-/
  • Some GNU/Linux distributions ship a mix between kodi and kodi-wayland: the /bin/kodi script already selects the right Kodi to launch if in a Wayland or X11 environment. So if we have a derivation which compiles both kodi + kodiPlainWayland, and "copy" ${kodiPlainWayland}/lib/kodi/kodi-wayland into ${kodi}/lib/kodi/, we should have something that is both native to X11 and Wayland!
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.

jtojnar
jtojnar reviewed Mar 26, 2019
View reviewed changes
pkgs/applications/video/kodi/default.nix
@@ -153,7 +158,7 @@ in stdenv.mkDerivation rec {
which
pkgconfig gnumake
autoconf automake libtool # still needed for some components. Check if that is the case with 18.0
];
] ++ lib.optional useWayland [ wayland-protocols ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really belong to nativeBuildInputs?

Copy link
Member Author

Choose a reason for hiding this comment

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

As I understand it, wayland-protocols are just xml files that describes the different protocols used in Wayland. They are converted to code at build time (usually with wayland-scanner for C/C++ headers).

@minijackson minijackson deleted the kodi-wayland branch April 10, 2019 11:25
@worldofpeace
Copy link
Contributor

@minijackson Any reason why you closed this?

@minijackson minijackson restored the kodi-wayland branch April 10, 2019 14:58
@minijackson
Copy link
Member Author

minijackson commented Apr 10, 2019 •
edited

Absolutely not, this was purely a mistake, sorry about that ^^'

Thank you for noticing and telling me!

@minijackson minijackson reopened this Apr 10, 2019
@worldofpeace
Copy link
Contributor

Absolutely not, this was purely a mistake, sorry about that ^^'

Thank you for noticing and telling me!

You're welcome 😄

I was considering that you closed this since it's been two weeks without a review.

Perhaps @cpages would be interested in reviewing, you can also ask in a discourse post or in
#nixos on freenode.

Else fails, I should be able to in a couple of days. (this was on my sights).

cpages
cpages reviewed Apr 10, 2019
View reviewed changes
pkgs/applications/video/kodi/default.nix Outdated
@@ -13,7 +13,8 @@
, libmpeg2, libsamplerate, libmad
, libogg, libvorbis, flac, libxslt
, lzo, libcdio, libmodplug, libass, libbluray
, sqlite, mysql, nasm, gnutls, libva, libdrm, wayland
, sqlite, mysql, nasm, gnutls, libva, libdrm
, wayland, wayland-protocols, waylandpp, libxkbcommon
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be optional now that wayland is optional

cpages
cpages reviewed Apr 10, 2019
View reviewed changes
pkgs/applications/video/kodi/default.nix Outdated
++ lib.optional vdpauSupport libvdpau;
++ lib.optional vdpauSupport libvdpau
++ lib.optional useWayland [
wayland waylandpp libxkbcommon.dev
Copy link
Contributor

@cpages cpages Apr 10, 2019 •
edited

Choose a reason for hiding this comment

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

Please add a comment if we cannot sort out needing .dev

@cpages
Copy link
Contributor

cpages commented Apr 10, 2019

I just did a quick review. I could not try it, but other than my minor suggestions, looks good to me. I'd go ahead and push this to have a working kodi for wayland. We can always try to have a mixed build later on.

Btw, I'll be away for a few days, so @worldofpeace you can merge when you think it's ok. Thanks!

@minijackson
Copy link
Member Author

I implemented @cpages 's suggestions (thanks for the review by the way!), rebased on master and everything still works like a charm!

@worldofpeace
Copy link
Contributor

@GrahamcOfBorg build kodi-wayland

worldofpeace
worldofpeace previously requested changes Apr 13, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Can you add wayland to the derivation name when useWayland is true?

@minijackson
Copy link
Member Author

Done!

@worldofpeace
Copy link
Contributor

@GrahamcOfBorg build kodi-wayland

sorry ofborg 😄

@worldofpeace worldofpeace dismissed their stale review April 14, 2019 13:19

resolved by author

worldofpeace
worldofpeace approved these changes Apr 15, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

I've built and ran this with wayland ❇️

@worldofpeace worldofpeace changed the title Kodi wayland kodi: add Wayland support Apr 15, 2019
@worldofpeace worldofpeace merged commit 5d8f4a1 into NixOS:master Apr 15, 2019
@worldofpeace
Copy link
Contributor

Thank you @minijackson

@minijackson minijackson deleted the kodi-wayland branch May 1, 2019 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpages cpages cpages left review comments

@jtojnar jtojnar jtojnar left review comments

@worldofpeace worldofpeace worldofpeace approved these changes

@domenkozar domenkozar Awaiting requested review from domenkozar

@edwtjo edwtjo Awaiting requested review from edwtjo

@peterhoeg peterhoeg Awaiting requested review from peterhoeg

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@minijackson @worldofpeace @cpages @jtojnar @GrahamcOfBorg