Skip to content

Conversation

@sudoAlphaX
Copy link
Contributor

add private-etc for mpv

Please verify is all functions are working and none are broken.

Also test hardware acceleration for various GPUs.

Let me know if i need to trim private-etc for other profiles which include mpv.profile such that they do not overlap.

Related: #6779

@sudoAlphaX
Copy link
Contributor Author

ill keep this as draft for now. I will be testing this with private-etc in my mpv.local.

Copy link
Collaborator

@kmk3 kmk3 left a comment

Choose a reason for hiding this comment

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

Let me know if i need to trim private-etc for other profiles which include
mpv.profile such that they do not overlap.

Yes, but please wait for #6783 first, which should make the overlap clearer.

Then, it would be good to use private-etc groups in mpv.profile as well.

See src/include/etc_groups.h for the groups.

Note that everything that is in the default group is automatically included and
can be removed from private-etc.

@sudoAlphaX sudoAlphaX marked this pull request as ready for review June 23, 2025 17:43
Copy link
Collaborator

@kmk3 kmk3 left a comment

Choose a reason for hiding this comment

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

To do:

  • Remove redundant mpv from private-etc in ani-cli.profile
  • Squash the commits

Please verify is all functions are working and none are broken.

Potential breakage is the main reason why whitelisting commands (such as
private-etc) aren't included in more profiles (especially for profiles that
are enabled in firecfg.config), so it is up to whoever proposes adding them to
investigate that (such as by checking paths and functionality in the man pages
and testing them).

That is, making sure that a profile keeps working is usually much harder than
adding an entry.

Also test hardware acceleration for various GPUs.

It seems currently broken on amd here regardless of this change, apparently due
to disable-common.inc.

When ignoring disable-common.inc it seems to work fine with the changes.

The amd paths seem to already be covered by the current items in @x11.

@sudoAlphaX
Copy link
Contributor Author

sudoAlphaX commented Jun 25, 2025

it is up to whoever proposes adding them to investigate that

I have been using this private-etc in my mpv.local for a few days now. I dont seem to have any problems.

It seems currently broken on amd here regardless of this change, apparently due
to disable-common.inc.

Shall I remove disable-common? But that doesn't seem resonable. I unfortunately do not have an AMD device right now. I will test and fix it in 2 weeks when I get access to an AMD machine.

Squash the commits

Do I have to squash them? Or will it be take care by squash and merge? If I have to squash them, should I include the ani-cli commit here or create a separate pr after this is merged?

@kmk3
Copy link
Collaborator

kmk3 commented Jun 25, 2025

it is up to whoever proposes adding them to investigate that

I have been using this private-etc in my mpv.local for a few days now. I dont
seem to have any problems.

I'm not an mpv power user so I don't know if there is any relatively common
workflow that could be broken by this, but the change in general seems fine to
me.

It seems currently broken on amd here regardless of this change, apparently
due to disable-common.inc.

Shall I remove disable-common? But that doesn't seem resonable. I
unfortunately do not have an AMD device right now. I will test and fix it in
2 weeks when I get access to an AMD machine.

For this PR, leave it as is.

Also note that the entries in this file overall are some of the most important
parts of the sandbox, so ideally the fix should involve only a small number of
lines being fixed/ignored rather than ignoring the entire file.

Squash the commits

Do I have to squash them? Or will it be take care by squash and merge? If I
have to squash them, should I include the ani-cli commit here or create a
separate pr after this is merged?

Commit the removal from ani-cli here, then squash everything.

It's good to squash them directly because:

  • It makes it clearer that the commits in question are all part of the same
    logical change
  • Sometimes PRs end up being merged using a normal merge, which makes the
    history more complex (especially when the PR branch already has a merge
    commit)

For an squashing example, see:

Also, if you tested something specific, it would be helpful to include that in
the commit message.

Example:

profiles: mpv: add private-etc

Tested with X and Y plugins.

Hardware acceleration works on Arch with mesa 1.2.3-1 and Intel HD Graphics
XXX.

Test command:

    $ firejail /usr/bin/mpv https://youtube.com/123

Related: #6779

@sudoAlphaX sudoAlphaX marked this pull request as draft June 27, 2025 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

2 participants