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

mkchromecast: init at 0.3.8.1 #57033

Merged
merged 1 commit into from Oct 19, 2020
Merged

mkchromecast: init at 0.3.8.1 #57033

merged 1 commit into from Oct 19, 2020

Conversation

@Shou
Copy link
Contributor

@Shou Shou commented Mar 7, 2019

Motivation for this change
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 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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@dtzWill
Copy link
Member

@dtzWill dtzWill commented Mar 7, 2019

LGTM! Thanks!

  • Hopefully they make a new release soon :3
  • Do you know how to get notifications working? Adding pygobject3 and libnotify to propagatedBuildInputs didn't seem enough. Not a deal-breaker but thought I'd ask 😁.

@Shou
Copy link
Contributor Author

@Shou Shou commented Mar 9, 2019

I haven't had much success in making notifications work. Someone has a minimal example that mimicks how it's done in mkchromecast but if you add --pure to that it emits gi.repository.GLib.Error: g-exec-error-quark: Failed to execute child process “dbus-launch” (No such file or directory) (8). The system tray icon still works, it only won't emit notifications during the Chromecast device scan as far as I can tell.

pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved
@Shou Shou force-pushed the init-mkchromecast branch from 157704c to f86f28c Mar 11, 2019
@Shou
Copy link
Contributor Author

@Shou Shou commented Mar 11, 2019

Should've addressed the CR feedback now. Let me know if there's anything I need to change

@Shou Shou force-pushed the init-mkchromecast branch 2 times, most recently from 2f6a13c to 8017502 Mar 17, 2019
pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved

format = "other";

# Relies on an old version (0.7.7) of PyChromecast unavailable in Nixpkgs.
Copy link
Member

@dotlambda dotlambda Mar 19, 2019

If the application only works with a certain version of a dependency, you should pin the latter using packageOverrides.

Copy link
Contributor Author

@Shou Shou Mar 24, 2019

That version isn't available in Nixpkgs at all and I assume packaging said old version is out of scope here – am I right to assume that?

Copy link
Member

@dotlambda dotlambda Mar 25, 2019

No need to package it separately, Just use packageOverrides as is e.g. done in pkgs/applications/misc/khard/default.nix.

Copy link
Member

@dotlambda dotlambda Apr 13, 2019

Since "master is actually more stable and supports the latest PyChromecast as well", we should definitely override the PyChromecast version as long as we're using version 0.3.8.1.

Copy link
Contributor Author

@Shou Shou Apr 18, 2019

Would it benefit us when the tests aren't really useful here? Could we just do this for the one targeting master?

pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved
alsaSupport = config.mkchromecast.alsaSupport or false;
systemTray = config.mkchromecast.systemTray or true;
ffmpegSupport = config.mkchromecast.ffmpegSupport or true;
youtubeSupport = config.mkchromecast.youtubeSupport or true;
Copy link
Member

@dotlambda dotlambda Mar 19, 2019

I'm not very familiar with the config system. However, I don't think these are useful as they can be achived using a simple override.

Copy link
Member

@dotlambda dotlambda Apr 13, 2019

Why did you just mark this as resolved?

Copy link
Contributor Author

@Shou Shou Apr 18, 2019

My bad! I did actually remove it, but some rebasing later it was back.

@Shou Shou force-pushed the init-mkchromecast branch from 8017502 to 775c81b Mar 25, 2019
@Shou
Copy link
Contributor Author

@Shou Shou commented Mar 25, 2019

Pushed changes following most feedback above.

Copy link
Member

@dotlambda dotlambda left a comment

With all this custom stuff this package needs, I think we'd be better off just packaging a version of mkchromecast from their master branch.

pkgs/applications/networking/mkchromecast/default.nix Outdated Show resolved Hide resolved
@Shou Shou force-pushed the init-mkchromecast branch from 775c81b to 7ebd48c Mar 25, 2019
@Shou
Copy link
Contributor Author

@Shou Shou commented Mar 25, 2019

How do we version/name a package based on a master branch? Do we add it to all-packages.nix as mkchromecastMaster and pin it against the specific commit master is at now? Edit: looks like that's the general behaviour based on some other packages' Nix derivations.

We could also mimic other packages by adding a master.nix, beside the stable release, but I do agree that we need to package master somehow because it's been a while since mkchromecast made a release.

@dotlambda
Copy link
Member

@dotlambda dotlambda commented Mar 25, 2019

We can just keep calling the attribute mkchromecast and set the version to unstable-YYYY-MM-DD, where the date is that of the commit used.

@Shou Shou force-pushed the init-mkchromecast branch from 7ebd48c to 220de4e Mar 25, 2019
@dotlambda
Copy link
Member

@dotlambda dotlambda commented Mar 25, 2019

However, if using master turns out to be just as complicated, I think we can go with it as it is right now.

@Shou Shou force-pushed the init-mkchromecast branch from 220de4e to 2a104e3 Mar 25, 2019
@Shou
Copy link
Contributor Author

@Shou Shou commented Mar 25, 2019

I'm happy to go ahead with this PR, and then give packaging master a try sometime later this week.

@dotlambda
Copy link
Member

@dotlambda dotlambda commented Mar 25, 2019

Alright, but you still have to change the version of PyChromecast used.

@Shou Shou force-pushed the init-mkchromecast branch 2 times, most recently from 8017502 to 770ad22 Apr 1, 2019
@Shou
Copy link
Contributor Author

@Shou Shou commented Apr 1, 2019

@dotlambda the tests here are actually I/O bound through testing an actual Chromecast Audio device ("CCA"), I don't think we want that, right? I've just added a further comment explaining why it's disabled.

(sorry for the silly diff, relevant change is on line 52)

@bobvanderlinden
Copy link
Contributor

@bobvanderlinden bobvanderlinden commented Apr 11, 2019

Good to see this PR already being submitted. Looks very good :D master is actually more stable and supports the latest PyChromecast as well. Since it has been more than a month since the request for a new version, I'd suggest using the latest master. You can find the changes in #59280.

@Shou Shou force-pushed the init-mkchromecast branch from 770ad22 to c369d14 Apr 18, 2019
@aanderse
Copy link
Member

@aanderse aanderse commented May 6, 2019

So can this PR be closed now as the package was already added? Any reason to keep it open @Shou?

@Shou
Copy link
Contributor Author

@Shou Shou commented Jun 3, 2019

@aanderse it hasn't been merged yet, the above PR was closed. I'm just waiting for feedback here

@aanderse
Copy link
Member

@aanderse aanderse commented Jun 9, 2019

@Shou Ah sorry, misread. I was just trying to triage.

@dotlambda seeing as you have had the most review on this PR is there anything outstanding from your perspective? Do you have any feedback for @Shou?

@aanderse
Copy link
Member

@aanderse aanderse commented Jul 13, 2019

@bobvanderlinden or @dtzWill Are either of you able to provide the requested feedback here?

@bobvanderlinden
Copy link
Contributor

@bobvanderlinden bobvanderlinden commented Jul 14, 2019

This version works for me! Tested on NixOS with older Chromecast. This version did not work for me. It raises an exception when casting videos or casting the screen. Command line for videos:

result/bin/mkchromecast --port 8080 --video -i myvideo.mp4

Command line for casting screen:

result/bin/mkchromecast --port 8080 --video --screencast --resolution 720p
[2019-07-14 18:48:26,269] ERROR in app: Exception on /stream [GET]
Traceback (most recent call last):
  File "/nix/store/4hn5lqcl115d90xik5v97nj1z246gq7h-python3.7-Flask-1.0.2/lib/python3.7/site-packages/flask/app.py", line 2292, in wsgi_app
    response = self.full_dispatch_request()
  File "/nix/store/4hn5lqcl115d90xik5v97nj1z246gq7h-python3.7-Flask-1.0.2/lib/python3.7/site-packages/flask/app.py", line 1815, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/nix/store/4hn5lqcl115d90xik5v97nj1z246gq7h-python3.7-Flask-1.0.2/lib/python3.7/site-packages/flask/app.py", line 1718, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/nix/store/4hn5lqcl115d90xik5v97nj1z246gq7h-python3.7-Flask-1.0.2/lib/python3.7/site-packages/flask/_compat.py", line 35, in reraise
    raise value
  File "/nix/store/4hn5lqcl115d90xik5v97nj1z246gq7h-python3.7-Flask-1.0.2/lib/python3.7/site-packages/flask/app.py", line 1813, in full_dispatch_request
    rv = self.dispatch_request()
  File "/nix/store/4hn5lqcl115d90xik5v97nj1z246gq7h-python3.7-Flask-1.0.2/lib/python3.7/site-packages/flask/app.py", line 1799, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/nix/store/1fb58idhqg2904imzcss7jid3nr1f0yn-mkchromecast-0.3.8.1/lib/mkchromecast/video.py", line 225, in stream
    process = Popen(command, stdout=PIPE, bufsize=-1)
  File "/nix/store/gbwx5sc4w0i0ipnmsa0nsk88svs983vl-python3-3.7.2/lib/python3.7/subprocess.py", line 775, in __init__
    restore_signals, start_new_session)
  File "/nix/store/gbwx5sc4w0i0ipnmsa0nsk88svs983vl-python3-3.7.2/lib/python3.7/subprocess.py", line 1522, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'ffmpeg': 'ffmpeg'

Might still be fine for now to init at 0.3.8.1 and hope that a new release will come soonish.

EDIT: After installing ffmpeg into my path it works fine. What I find interesting is that my PR didn't have this problem. I also tried updating pychromecast to 3.2.0, but that resulted in the same exception. I guess this issue was resolved in master (compared to 0.3.8.1). For 0.3.8.1 I'd say adding ffmpeg to the propagated builds is required for this tool to work correctly.

@aanderse
Copy link
Member

@aanderse aanderse commented Jul 15, 2019

Thanks @bobvanderlinden! @Shou sound good?

@Shou Shou force-pushed the init-mkchromecast branch from c369d14 to ebe40a6 Jul 17, 2019
@Shou
Copy link
Contributor Author

@Shou Shou commented Jul 17, 2019

@aanderse @bobvanderlinden have done that now

@aanderse
Copy link
Member

@aanderse aanderse commented Jul 21, 2019

Thanks @Shou! @bobvanderlinden Hopefully you'll be able to test and merge this at some point... unfortunately I don't have the hardware available currently to do so.

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Mar 8, 2020

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/110

@stale
Copy link

@stale stale bot commented Sep 4, 2020

Hello, I'm a bot and I thank you in the name of the community for your contributions.

Nixpkgs is a busy repository, and unfortunately sometimes PRs get left behind for too long. Nevertheless, we'd like to help committers reach the PRs that are still important. This PR has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

If this is still important to you and you'd like to remove the stale label, we ask that you leave a comment. Your comment can be as simple as "still important to me". But there's a bit more you can do:

If you received an approval by an unprivileged maintainer and you are just waiting for a merge, you can @ mention someone with merge permissions and ask them to help. You might be able to find someone relevant by using Git blame on the relevant files, or via GitHub's web interface. You can see if someone's a member of the nixpkgs-committers team, by hovering with the mouse over their username on the web interface, or by searching them directly on the list.

If your PR wasn't reviewed at all, it might help to find someone who's perhaps a user of the package or module you are changing, or alternatively, ask once more for a review by the maintainer of the package/module this is about. If you don't know any, you can use Git blame on the relevant files, or GitHub's web interface to find someone who touched the relevant files in the past.

If your PR has had reviews and nevertheless got stale, make sure you've responded to all of the reviewer's requests / questions. Usually when PR authors show responsibility and dedication, reviewers (privileged or not) show dedication as well. If you've pushed a change, it's possible the reviewer wasn't notified about your push via email, so you can always officially request them for a review, or just @ mention them and say you've addressed their comments.

Lastly, you can always ask for help at our Discourse Forum, or more specifically, at this thread or at #nixos' IRC channel.

@stale stale bot added the 2.status: stale label Sep 4, 2020
@bobvanderlinden
Copy link
Contributor

@bobvanderlinden bobvanderlinden commented Sep 4, 2020

It would be a shame of this wasn't merged.

@stale stale bot removed the 2.status: stale label Sep 4, 2020
@bobvanderlinden
Copy link
Contributor

@bobvanderlinden bobvanderlinden commented Sep 4, 2020

@austinbutler
Copy link
Contributor

@austinbutler austinbutler commented Sep 4, 2020

Tested this with my Chromecast Audio but ran into what appears to be this issue: muammar/mkchromecast#308. Seems like it's probably just an issue with mkchromecast itself, though. It does run and connect to the Chromecast at least.

@marsam marsam force-pushed the init-mkchromecast branch from ebe40a6 to 3d10184 Oct 19, 2020
@marsam marsam force-pushed the init-mkchromecast branch from 3d10184 to f6da38a Oct 19, 2020
@marsam marsam merged commit ad3234b into NixOS:master Oct 19, 2020
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants