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

- add gst-plugins-base 1.19.1 #7142

Merged
merged 13 commits into from
Sep 27, 2021
Merged

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Sep 1, 2021

Specify library name and version: gst-plugins-base/1.19.1

recipe from bincrafters: https://github.com/bincrafters/community/tree/main/recipes/gst-plugins-base
I'd like to continue multimedia libraries journey a bit
gstreamer is quite popular, and it would be nice to have its plug-ins (base, good, bad, ugly) and required for several libraries (Qt::Multimedia, wxWidgets, etc.).


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@SSE4 SSE4 requested a review from uilianries September 1, 2021 15:42
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@SSE4 SSE4 closed this Sep 1, 2021
@SSE4 SSE4 reopened this Sep 1, 2021
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@SSE4 SSE4 closed this Sep 2, 2021
@SSE4 SSE4 reopened this Sep 2, 2021
@SSE4 SSE4 closed this Sep 6, 2021
@SSE4 SSE4 reopened this Sep 6, 2021
@conan-center-bot

This comment has been minimized.

recipes/gst-plugins-base/all/conanfile.py Outdated Show resolved Hide resolved
recipes/gst-plugins-base/all/conanfile.py Show resolved Hide resolved
recipes/gst-plugins-base/all/conanfile.py Outdated Show resolved Hide resolved
recipes/gst-plugins-base/all/test_package/conanfile.py Outdated Show resolved Hide resolved
recipes/gst-plugins-base/all/test_package/CMakeLists.txt Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

uilianries
uilianries previously approved these changes Sep 9, 2021
del self.options.fPIC
del self.settings.compiler.libcxx
del self.settings.compiler.cppstd
self.options['gstreamer'].shared = self.options.shared
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be changed to a check in validate?

if self.options.shared and not self.options["gstreamer"].shared:
    raise ConanInvalidConfiguration("The options gstreamer:shared and {}:shared must be the same".format(self.name))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. Thanks.
But this line overrides user arguments.
What happens if I build with the following arguments?

conan create . gst-plugins-base/1.19.1@ -o gstreamer:shared=True -o gst-plugins-base:shared=False
conan create . gst-plugins-base/1.19.1@ -o gstreamer:shared=False -o gst-plugins-base:shared=True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the bad things happen for sure.
GLib is designed to be present only once within an entire address space.
mixing shared/static leads to undefined behavior (e.g. CI hands).
literally, we run into the issue https://gitlab.freedesktop.org/gstreamer/gst-build/-/issues/133
e.g. locally I get these errors:

objc[21642]: Class GNotificationCenterDelegate is implemented in both /Users/sse4/.conan/data/gstreamer/1.19.1/_/_/package/456f111003e278e1a2f9a487e4964b0dec0b5ce3/lib/libgstnet-1.0.0.dylib (0x100a663b8) and /Users/sse4/.conan/data/gst-plugins-base/1.19.1/_/_/package/15f899debcee0aa9e4e9a72e52b66594fa545dbe/lib/libgstrtsp-1.0.0.dylib (0x1004c2378). One of the two will be used. Which one is undefined.

and many errors of that type:

(process:3125): GLib-GObject-CRITICAL **: 15:57:26.899: g_param_spec_pool_lookup: assertion 'pool != NULL' failed

these errors indicate two instances of glib exist in the address place.

the bad things also happen with conan side here...
I gonna submit new bug, but I doubt it will be ever fixed (at least in 1.x).
if I request the following build:

conan create . gst-plugins-base/1.19.1@ \
 -o gst-plugins-base:shared=True \
 -o gstreamer:shared=True \
 -o glib:shared=True \

what do you think happens?
I am requesting all glib, gstreamer and gst-plugins-base to be shared libraries.
does conan guarantee that all of them will be shared?
nope, it surprisingly consumes gstreamer:shared=True linked to glib:shared=False.
and there seems to be no way to override it from command line?

why does it happen?
both packages gstreamer:shared=True with glib:shared=True and gstreamer:shared=True with glib:shared=False have the same package id!
for me, running on Mac:

conan info gstreamer/1.19.1 -o gstreamer:shared=True -o glib:shared=False
conan info gstreamer/1.19.1-o gstreamer:shared=True -o glib:shared=True

I get the exactly same result:

gstreamer/1.19.1
    ID: 5a1e3781444d60ee53a8d7e762ece8f559cf15ca

so the options glib:shared is erased from the package id of the gstreamer package.

are these two packages supposed to have the same package id? I don't think so.
the first one embeds glib objects into the gstreamer DLL.
the second one links gstreamer DLL into glib DLL.
I think that's entirely different package with a different behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the problem you're describing the need for full transitive package ids? default_package_id_mode and full_transitive_package_id?
But you're right, all shared libraries built by c3i have all static libraries built-in. This is really awkward.

My comment was about the recipe overriding user options, which is a no no imho.

Perhaps conan needs apost_validate method.
A method in which a recipe can check the complete dependency tree.
Then , the following can be added to the glib recipe:

def post_validate(self):
    if not self.options.shared:
        consumers = self.tree.get_consumers(self.name)
        shared_consumers = [consumer for consumer in consumers if getattr(consumer.options, "shared", False) == True]
        if len(shared_consumers) > 1:
            raise ConanInvalidConfiguration("glib is used by more then 1 library that is built as a shared library. This will lead to undefined behavior.")

Copy link
Contributor Author

@SSE4 SSE4 Sep 12, 2021

Choose a reason for hiding this comment

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

I don't think we need post_validate.
validate already runs for the final dependency tree, after all overrides computed.

Another important difference with the configure() method, is that validate() is evaluated after 
the graph has been computed and the information has been propagated downstream. 
So the values used in validate() are guaranteed to be final real values, while values at configure() time are not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. I thought about it too late.

tools.remove_files_by_mask(self.package_folder, "*.pdb")

def package_info(self):
gst_plugin_path = os.path.join(self.package_folder, "lib", "gstreamer-1.0")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is missing setting the names for the pkg_config generator.
It looks like a lot of libraries are built.
Does this need components?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to be honest, no idea.
I've taken bincrafters recipe which didn't have this.
for me it worked, also CI passes, so not sure if anything else is required.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't test pkg_config on CI.
It will be some obscure dependency that will flag an error.
Probably best to add a FIXME: missing pkg_config name(s) + components.

@@ -0,0 +1,44 @@
#include <gst/gst.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this source can be easily converted to c.
It would also test whether the cppstd library is linked correctly, if needed by a dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it can be converted, but it needs some minor modifications (e.g. replace std::iostream with printf)

@conan-center-bot
Copy link
Collaborator

All green in build 16 (ee7480c04c0d38733e2428acea1750b21ff339fc):

  • gst-plugins-base/1.19.1@:
    All packages built successfully! (All logs)

@SSE4 SSE4 requested a review from uilianries September 17, 2021 06:23
Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

I'm ok with this first iteration. There will always be some details to improve, and some underlying issues for the long term...

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.

6 participants