-
Notifications
You must be signed in to change notification settings - Fork 584
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
New profile: lobster #5706
New profile: lobster #5706
Conversation
(Rebased to master to fix CI) Just to confirm, is this https://github.com/justchokingaround/lobster? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some checks needed for potential hardening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this script is mostly a wrapper for mpv and yet the profile is more
restrictive than mpv.profile.
For example, it does not include paths which may be needed by mpv plugins:
include allow-lua.inc
whitelist /usr/share/lua
whitelist /usr/share/lua*
And it contains machine-id
, which AFAIK may break pulseaudio.
Some of the extra restrictions make sense to me given the more focused
scenario, such as nodvd
and include disable-xdg.inc
.
How about keeping them and making this a redirect to mpv.profile?
To keep it in sync with mpv.profile and avoid duplication.
yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, just a few nits.
i also commented out machine-id for pulseaudio. |
That would indeed be a nicer way to do things, I agree. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on principal. Can you try @kmk3's suggestion to refactor this as mpv redirect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid including globals.local twice. Format to achieve that is in my comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice to have this as a redirect profile now, congrats 👍.
all in, thanks! |
Note: mpv itself does not modify anything in ~/.config/mpv as far as I know, in which case it does not need a read-write entry. Relates to netblue30#5706 netblue30#5707 netblue30#5710.
it's a shell script for streaming