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

media-libs/libva: Fix circular dependency with mesa #29900

Closed
wants to merge 4 commits into from

Conversation

DarkDefender
Copy link
Contributor

@DarkDefender DarkDefender commented Mar 2, 2023

This removes the GLX backend to drop the "virtual/opengl" dependency. Without removing this, it would pull in mesa which in turn would pull in libva if vaapi support was turned on.

Removing the GLX backend doesn't seem to have any practical downsides, even under X11, as the EGL backend seems to be used even if libva were compiled with GLX support.

Mesa is currently unconditionally built with EGL support, so there shouldn't be any situation where an end user can end up needing the GLX backend.

However using EGL instead of GLX breaks the dependency loop as it does not need any headers from mesa at compile time.
(The libraries does also not directly link to any opengl libs)

@mattst88 I'm unsure if you are the maintainer now?
This commit is a bit fuzzy about this detail: 1ef3e2a

@gentoo-bot
Copy link

Pull Request assignment

Submitter: @DarkDefender
Areas affected: ebuilds
Packages affected: media-libs/libva

media-libs/libva: @gentoo/va-api

Linked bugs

No bugs to link found. If your pull request references any of the Gentoo bug reports, please add appropriate GLEP 66 tags to the commit message and request reassignment.

If you do not receive any reply to this pull request, please open or link a bug to attract the attention of maintainers.


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels Mar 2, 2023
@DarkDefender
Copy link
Contributor Author

Note that this is just a proposed solution, so I am open for discussing this as I do agree that this might be a bit heavy handed.
(However it does simplify things quite a bit)

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-03-02 16:03 UTC
Newest commit scanned: 241c0ac
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/f815e6490b/output.html

@mattst88
Copy link
Contributor

mattst88 commented Mar 2, 2023

@mattst88 I'm unsure if you are the maintainer now?
This commit is a bit fuzzy about this detail: 1ef3e2a

Yes, see https://wiki.gentoo.org/wiki/Project:VA-API

This removes the GLX backend to drop the "virtual/opengl" dependency. Without removing this, it would pull in mesa which in turn would pull in libva if vaapi support was turned on.

I'm willing to give it a try, but we shouldn't make this change in stable ebuilds. Let's make a 2.17.0-r1 that has this change. Same thing for reverse dependencies; we cannot just modify dependencies in stable ebuilds for a few reasons, not the least of which is that they won't be rebuilt and will prevent installation of the new libva.

But I don't understand how the patch makes that change exactly. It looks like it removes IUSE=opengl and leaves IUSE=X in place? That is, if this stops building the GLX backend, what are the dependencies on libX11, libXext, et al still doing there?

Dropping IUSE=opengl is a good thing to do—it's a dummy USE flag that I've never finished removing completely.

@DarkDefender
Copy link
Contributor Author

DarkDefender commented Mar 2, 2023

I'm willing to give it a try, but we shouldn't make this change in stable ebuilds. Let's make a 2.17.0-r1 that has this change. Same thing for reverse dependencies; we cannot just modify dependencies in stable ebuilds for a few reasons, not the least of which is that they won't be rebuilt and will prevent installation of the new libva.

Ah yeah, that is a much more sensible solution. I'll revert the changes to everything and just drop the opengl useflag in new and live ebuilds.

But I don't understand how the patch makes that change exactly. It looks like it removes IUSE=opengl and leaves IUSE=X in place? That is, if this stops building the GLX backend, what are the dependencies on libX11, libXext, et al still doing there?

If I understand everything correctly, then the x11/wayland backends are used to tie a vaapi surface to a native X11 or Wayland surface. So the GLX backend provided a way to tie a GLX OpenGL surface to the VAAPI output.
At least that is what I get from looking at the header files:
https://github.com/intel/libva/blob/master/va/va_x11.h
https://github.com/intel/libva/blob/master/va/wayland/va_wayland.h

So in case the a program is not using a OpenGL surface to display the vaapi output, we need these backends.
You can see that the specific x11 libs are pulled in here:
https://github.com/intel/libva/blob/master/meson.build#L82

I'm not 100% certain I interpreted this correctly though.

@DarkDefender
Copy link
Contributor Author

@mattst88 I'm guessing that we just ignore the CI warnings?
Or is there any special "no, it is fine" action I can do?

@ConiKost
Copy link
Contributor

ConiKost commented Mar 7, 2023

Yes, they need to be fixed. Since you dropped the opengl use flag, those two failing packages in CI need to know, how to handle this.

See: https://devmanual.gentoo.org/general-concepts/dependencies/index.html

If a dependency is introducing or removing a USE flag in a new package version, a (+) or (-) may be added to the use-dependency specification to define a default value in case the flag does not exist in the target package. The (+) indicates that the missing flag is assumed to be enabled, (-) the opposite.

I guess, that you want opengl(+).

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-03-07 15:48 UTC
Newest commit scanned: 3324920
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/3afd433e00/output.html

@DarkDefender
Copy link
Contributor Author

DarkDefender commented Mar 7, 2023

@ConiKost thanks for the heads up!
It seems to be fine now :)

And yes, opengl(+) is correct as opengl support is always built into the package now.
It simply does not have a hard build time opengl dependency anymore.

@ConiKost
Copy link
Contributor

ConiKost commented Mar 7, 2023

Yep, then opengl(+) is the right way :-)
But since you added all into one single commit, could you please split it up? mythtv, kodi and xine-lib should have their own seperate commits.

@DarkDefender
Copy link
Contributor Author

DarkDefender commented Mar 7, 2023

Just to be clear you want one commit for the libva change. And a commit each of the other packages.
So one for all changes to xine-lib, an other for kodi etc?

@ConiKost
Copy link
Contributor

ConiKost commented Mar 7, 2023

Yes, exactly :-)

This removes the GLX backend to drop the "virtual/opengl" dependency.
Without removing this, it would pull in mesa which in turn would pull in
libva if vaapi support was turned on.

Removing the GLX backend doesn't seem to have any practical downsides,
even under X11, as the EGL backend seems to be used even if libva were
compiled with GLX support.

Signed-off-by: Sebastian Parborg <darkdefende@gmail.com>
@DarkDefender
Copy link
Contributor Author

Done! :)

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-03-07 17:13 UTC
Newest commit scanned: 2def471
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/9fbf47584d/output.html

@ConiKost
Copy link
Contributor

ConiKost commented Mar 8, 2023

@mattst88 lgtm. Wdyt?

@mattst88
Copy link
Contributor

mattst88 commented Mar 8, 2023

diff --git a/media-libs/xine-lib/xine-lib-1.2.13.ebuild b/media-libs/xine-lib/xine-lib-1.2.13.ebuild
index a7cce97ce1ac7..5440556bc5c9b 100644
--- a/media-libs/xine-lib/xine-lib-1.2.13.ebuild
+++ b/media-libs/xine-lib/xine-lib-1.2.13.ebuild
@@ -81,7 +81,7 @@ RDEPEND="
 		media-libs/freetype:2
 	)
 	v4l? ( media-libs/libv4l )
-	vaapi? ( media-libs/libva:=[opengl,X] )
+	vaapi? ( media-libs/libva:=[opengl(+),X] )
 	vcd? (
 		>=media-video/vcdimager-0.7.23
 		dev-libs/libcdio:=[-minimal]

We can't just modify this in place -- there's nothing that will trigger a rebuild for it, so if it's already installed it'll continue to depend on libva[opengl]. You have to make revision bumps for anything that is updated.

@DarkDefender
Copy link
Contributor Author

DarkDefender commented Mar 8, 2023

Ok, then how do we solve the CI warnings?
If I do a revision bump the flag doesn't need to be there at all because even before this change, you couldn't disable opengl support (even if the flag was turned off).
Therefore the opengl(+) is superfluous and doesn't need to be there at all.

So to me it seems like the only way left would be to remove the old ebuilds completely.

@mattst88
Copy link
Contributor

mattst88 commented Mar 8, 2023

Sorry, I missed the CI warnings so I've tried to recreate them—

MissingUseDepDefault: version 32.0-r2: DEPEND="media-libs/libva:=[opengl]": USE flag 'opengl' missing from packages: [ media-libs/libva-2.17.0-r1, media-libs/libva-9999 ]

For this, it's okay to revbump the stable ebuilds if we're confident that the functionality (previously) enabled by libva's USE=opengl is not required or affecting anything.

Otherwise, it's fine to leave the stable ebuilds alone and ignore the warning with the knowledge that we just need to stabilize xine-lib, kodi, and mythtv before stabilizing libva that doesn't have IUSE=opengl.

@DarkDefender
Copy link
Contributor Author

From my tests it seems like we should be able to just revbump the stable builds and have no issues.
However I can't be 100% sure of this. (But I feel 99% confident).

I guess it is your call on what you want to do.
Should I revdump or just remove the opengl(+) changes?

Signed-off-by: Sebastian Parborg <darkdefende@gmail.com>
Signed-off-by: Sebastian Parborg <darkdefende@gmail.com>
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-03-15 15:39 UTC
Newest commit scanned: 22f24a0
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/ad432f3b9a/output.html

@DarkDefender
Copy link
Contributor Author

DarkDefender commented Mar 15, 2023

@mattst88 I did the revbump as you suggested. Now no ebuild depend on the opengl flag for libva.
There shouldn't be any noticeable change for the enduser.

If there are any issues, simply just rolling back to an older version than libva-2.17.0-r1 should solve it. Rolling back or reverting the changes for any of the other packages shouldn't been needed as the opengl useflag was just a dummy useflag.

So if any unforeseen issues happen, all that should be needed is to revert the disabling of the glx backend in the libva ebuilds.

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-03-15 15:48 UTC
Newest commit scanned: 2be5bfe
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/932fca92c7/output.html

@mattst88
Copy link
Contributor

Sorry I haven't had time to get back to this.

@DarkDefender
Copy link
Contributor Author

No problem. I've been kinda busy too, so I know the feeling.
Do you think you will have time in the coming weeks?

Signed-off-by: Sebastian Parborg <darkdefende@gmail.com>
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-04-03 15:24 UTC
Newest commit scanned: 3aa2197
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/47691d7899/output.html

@DarkDefender
Copy link
Contributor Author

@mattst88 any light at the end of the tunnel? 😉

@DarkDefender
Copy link
Contributor Author

@ConiKost perhaps you can take over the review and give the final greenlight as it seems like Mats will not have time in the near future.
I'm pushing for this as this circular dependency regularly prevents me from cleanly doing bigger emerges with changed use flags.

@DarkDefender
Copy link
Contributor Author

@mattst88 thanks for merging! Sorry for all the nagging...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits.
Projects
None yet
5 participants