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.

Sign up for GitHub

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

MythTV 29.1 -> 30.0 #71002

Closed
wants to merge 1 commit into from
Closed

MythTV 29.1 -> 30.0 #71002

wants to merge 1 commit into from

Conversation

LouisDK1
Copy link
Contributor

Restoration of this pull request: #67191

Motivation for this change

#54596

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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

Jonathan Rudenberg jonathan@titanous.com

@LouisDK1 LouisDK1 changed the title MythTV 29.1 -> 30.0 + option to build with or without xnvctrl support MythTV 29.1 -> 30.0 Oct 11, 2019
@LouisDK1
Copy link
Contributor Author

I'll have to path some unpure stuff starting at line 6669 to prevent the build from failing if build with xnvctrl enabled. Will look into this tomorrow.

@LouisDK1
Copy link
Contributor Author

I have to combine these two call packages in all-packages.nix:
mythtv = libsForQt5.callPackage ../applications/video/mythtv { };

mythtv = callPackage ../applications/video/mythtv { libXNVCtrl = linuxPackages.nvidia_x11.settings.libXNVCtrl; };

However I don't know how to do this.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/multiple-callpackage-for-one-package/4373/1

@jonringer
Copy link
Contributor

{ libXNVCtrl = linuxPackages.nvidia_x11.settings.libXNVCtrl; }; is just an attribute set (think dictionary in other languages) that is being passed to a function. Just fill the parameter with the values you need.

mythtv = libsForQt5.callPackage ../applications/video/mythtv {
  libXNVCtrl = linuxPackages.nvidia_x11.settings.libXNVCtrl;
 };

@ofborg ofborg bot requested a review from titanous October 15, 2019 10:57
@LouisDK1
Copy link
Contributor Author

I'll commit some final changes within an hour.

@LouisDK1
Copy link
Contributor Author

I'll have to correct a patch.

@LouisDK1
Copy link
Contributor Author

building '/nix/store/z7p5knmsrif1l9zf1m46h91ng3zpjnnn-mythtv-30.0.drv'...
unpacking sources
unpacking source archive /nix/store/fkm3d0yvrd4cq5y0aam4lsm32j6hqf2z-source
source root is source/mythtv
patching sources
applying patch /nix/store/2hsa4z1kqsk53lcg405pj2hq47yxlkrn-004-exiv2.patch
patching file libs/libmythmetadata/imagemetadata.cpp
applying patch /nix/store/m6jzly65il7xkpdkh0h522y2p7fwj9gf-disable_xnvctrl_detection.patch
patching file configure
Hunk #1 FAILED at 6641.
1 out of 1 hunk FAILED -- saving rejects to file configure.rej
builder for '/nix/store/z7p5knmsrif1l9zf1m46h91ng3zpjnnn-mythtv-30.0.drv' failed with exit code 1

I'm a bit unsure if I have disabled enough stuff in disable_xnvctrl_detection.patch - is enough to comment line from line 6644 (if enabled xnvctrl; then) or should I start at line 6640 (if enabled x11; then) ?

@LouisDK1
Copy link
Contributor Author

@jonringer Hi. Could you look at this updated pull request? Thanks.

jonringer
jonringer reviewed Oct 26, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

some of these are not launching right, applying the changes should fix this, IIRC

nix-shell:/home/jon/.cache/nix-review/pr-71002]$ ./results/mythtv/bin/mythwelcome
qt.qpa.plugin: Could not find the Qt platform plugin "xcb" in ""
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

Aborted (core dumped)

pkgs/applications/video/mythtv/default.nix Outdated Show resolved Hide resolved
pkgs/applications/video/mythtv/default.nix Outdated
@@ -1,40 +1,38 @@
{ stdenv, fetchFromGitHub, which, qtbase, qtwebkit, qtscript, xlibsWrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{ stdenv, fetchFromGitHub, which, qtbase, qtwebkit, qtscript, xlibsWrapper
{ stdenv, mkDerivation, fetchFromGitHub, which, qtbase, qtwebkit, qtscript, xlibsWrapper

pkgs/applications/video/mythtv/patches/fix-os-detection.patch Outdated
# Bah. Suse linux doesn't have xnvctrl.
- . /etc/os-release
- case $ID in
+ case nix in
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure how i feel about this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made another fix which might be a better way to patch it.

Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove this file now?

@LouisDK1
Copy link
Contributor Author

@jonringer Hi. Could you look at this updated pull request yet again? Thanks.

@rycee
Copy link
Member

rycee commented Oct 26, 2019

@GrahamcOfBorg build mythtv

rycee
rycee approved these changes Oct 26, 2019
View reviewed changes
Copy link
Member

@rycee rycee left a comment

Choose a reason for hiding this comment

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

Builds OK for me and the binaries I tried worked fine for me. E.g. mythwelcome.

pkgs/applications/video/mythtv/disable-os-detection.patch
if enabled xnvctrl; then
- case $target_os in
- linux)
+# case $target_os in
Copy link
Member

Choose a reason for hiding this comment

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

An option would be to simply delete the commented lines instead of commenting them out.

jonringer
jonringer approved these changes Oct 26, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

nix-review passes on NixOS
diff LGTM
executables seem to work

[2 built, 0.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/71002
1 package were build:
mythtv

I like @rycee 's suggestion

rycee pushed a commit that referenced this pull request Oct 27, 2019
@LouisDK1 @rycee
mythtv: 29.1 -> 30.0 (#71002)
2896f00
@rycee
Copy link
Member

rycee commented Oct 27, 2019

Rebased to master in 2896f00.

@rycee rycee closed this Oct 27, 2019
@LouisDK1 LouisDK1 deleted the mythtv branch October 27, 2019 09:01
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Oct 29, 2019
@LouisDK1 @dtzWill
mythtv: 29.1 -> 30.0 (NixOS#71002)
701eb7c 
(cherry picked from commit 2896f00)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rycee rycee rycee approved these changes

@jonringer jonringer jonringer approved these changes

@titanous titanous Awaiting requested review from titanous

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@LouisDK1 @nixos-discourse @jonringer @rycee