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

apt: Fix matching gstreamer pkgs where the only modifier is ()(64bit) #810

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aleasto
Copy link
Contributor

@aleasto aleasto commented Nov 13, 2024

When the list of GstCaps specified is empty, and the only modifier we're given is ()(64bit), our regex eats the outer parenthesis so we're left with ")(64bit" while trying to match "()(64bit".

This change would introduce a trailing comma when the list of specified GstCaps is instead not empty, which gst_caps_from_string today can handle, but for abundance of caution let's avoid it.

Test cases:

pkcon what-provides "gstreamer1(decoder-audio/mpeg)(mpegversion=4)()(64bit)"
pkcon what-provides "gstreamer1(decoder-audio/mpeg)(mpegversion=4)"
pkcon what-provides "gstreamer1(decoder-video/x-h265)()(64bit)"
pkcon what-provides "gstreamer1(decoder-video/x-h265)"

When the list of GstCaps specified is empty, and the only modifier we're given
is ()(64bit), our regex eats the outer parenthesis so we're left with ")(64bit"
while trying to match "()(64bit".

This change would introduce a trailing comma when the list of specified GstCaps
is instead not empty, which gst_caps_from_string today can handle, but for
abundance of caution let's avoid it.

Test cases:
pkcon what-provides "gstreamer1(decoder-audio/mpeg)(mpegversion=4)()(64bit)"
pkcon what-provides "gstreamer1(decoder-audio/mpeg)(mpegversion=4)"
pkcon what-provides "gstreamer1(decoder-video/x-h265)()(64bit)"
pkcon what-provides "gstreamer1(decoder-video/x-h265)"
@3v1n0
Copy link

3v1n0 commented Nov 20, 2024

Looks good to me. Code seems a bit fragile though (not as per your change), so wondering if we can add some testing to ensure we're not breaking anything now or in the future.

@aleasto
Copy link
Contributor Author

aleasto commented Nov 27, 2024

Added tests

Copy link

@3v1n0 3v1n0 left a comment

Choose a reason for hiding this comment

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

Lovely, from the APT side of things it looks good and thanks a lot for adding tests so that we can easily maintain this complex piece of parsing code.

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.

2 participants