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

feat: check for gstreamer or pipewire libraries before setting them #24

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

soumyaDghosh
Copy link

This PR allows snaps to use their own set of pipewire or gstreamer libraries.

@kenvandine kenvandine requested review from sergiusens and sergio-costas and removed request for sergiusens March 18, 2024 22:43
@sergio-costas
Copy link
Collaborator

@soumyaDghosh What is the problem that this patch fixes? In which cases can those variables be previously defined?

@soumyaDghosh
Copy link
Author

In cases when the snap is building gstreamer or pipewire from and shipping within the snap, for eg, firefox, chromium, clapper

@sergiusens
Copy link
Collaborator

@kenvandine I don't think I am the right person to review this

@nteodosio
Copy link
Contributor

This needs to be merged to fix https://launchpad.net/bugs/2074358. @jhenstridge or @sergio-costas, can you help reviewing this?

common/desktop-exports Outdated Show resolved Hide resolved
Copy link
Collaborator

@sergio-costas sergio-costas left a comment

Choose a reason for hiding this comment

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

As Nathan commented, each environment variable should be checked independently.

common/desktop-exports Outdated Show resolved Hide resolved
@soumyaDghosh soumyaDghosh force-pushed the main branch 2 times, most recently from 9a45a96 to 831333e Compare July 29, 2024 13:36
This PR allows snaps to use their own set of pipewire or gstreamer libraries.
@nteodosio
Copy link
Contributor

+1, thank you!

@sergio-costas sergio-costas self-requested a review November 27, 2024 11:19
Copy link
Collaborator

@sergio-costas sergio-costas left a comment

Choose a reason for hiding this comment

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

Ok, it makes sense.

@sergio-costas
Copy link
Collaborator

@nteodosio I was doing some maintenance checks and found this awaiting for my approval... Is this still required?

@nteodosio
Copy link
Contributor

nteodosio commented Nov 27, 2024

I'd say yes. I worked around it in Chromium, but this is sure to bite someone, somewhen if not merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants