- [gstreamer] use full_package_mode for GLib #7734

SSE4 · 2021-10-17T13:15:16Z

long story, please read carefully:
#7142 (comment)
conan-io/conan#9712

TLDR:

GLib must exist in address space only once, otherwise UB (may crash, hang, as we see in - [gstreamer] bump glib to 2.70.0 #7634)
it means the only valid combinations of options are:
2.1. glib:shared=False, gstreamer:shared=False, gst-plugins-*:shared=False
2.2. glib:shared=True, gstreamer:shared=True, gst-plugins-*:shared=True
2.3. everything else is unsupported and should be discarded in validate
that complicates an issue more glib:shared=False is not supported on Windows (read, if you're interested: https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1655 https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1316)
that CCI currently builds are:
4.1. all static (default) - *:shared=False
4.2. <current recipe>:shared=True + all the dependencies static (default)
CONSEQUENCE 1: there will be no shared GStreamer builds on CCI, they must be built from sources (shared will be rejected because it's a mixture of static + shared)
CONSEQUENCE 2: there will be no Windows GStreamer builds on CCI, they must be built from sources (static will be rejected because static Windows GLib is not supported, shared will be rejected for the reason specified in the previous point)

being pragmatic, it's probably too much to reject these builds and require to always build from sources.
OTOH these builds are technically invalid anyway, and shouldn't be used by anyone, since they contain UB.
I am very tentative on these changes, so it's a draft for now, let's discuss and exchange opinions/arguments.

Specify library name and version: gstreamer/all

This is also a good place to share with all of us why you are submitting this PR (specially if it is a new addition to ConanCenter): is it a dependency of other libraries you want to package? Are you the author of the library? Thanks!

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.

Signed-off-by: SSE4 <[email protected]>

SpaceIm · 2021-10-17T13:34:15Z

CONSEQUENCE 1: there will be no shared GStreamer builds on CCI, they must be built from sources (shared will be rejected because it's a mixture of static + shared)
CONSEQUENCE 2: there will be no Windows GStreamer builds on CCI, they must be built from sources (static will be rejected because static Windows GLib is not supported, shared will be rejected for the reason specified in the previous point)

Therefore I advice to:

override default shared option of gstreamer to True if Windows (in config_options())
force glib shared in configure() if gstreamer shared.

It's important for CCI to provide binaries and a valid package id for the default options, otherwise it contaminates all downstream recipes.

recipes/gstreamer/all/conanfile.py

Co-authored-by: SpaceIm <[email protected]>

jgsogo · 2021-10-18T07:48:23Z

it means the only valid combinations of options are:
2.1. glib:shared=False, gstreamer:shared=False, gst-plugins-*:shared=False
2.2. glib:shared=True, gstreamer:shared=True, gst-plugins-*:shared=True
2.3. everything else is unsupported and should be discarded in validate

What about glib:shared=True, gstreamer:shared=xxxx, gst-plugins-*:shared=xxxx? Is it not enough glib:shared=True?

I mean, if glib is static, then everything needs to be static; but if it is shared there is no problem downstream.

SSE4 · 2021-10-18T09:00:53Z

it means the only valid combinations of options are:
2.1. glib:shared=False, gstreamer:shared=False, gst-plugins-*:shared=False
2.2. glib:shared=True, gstreamer:shared=True, gst-plugins-*:shared=True
2.3. everything else is unsupported and should be discarded in validate

What about glib:shared=True, gstreamer:shared=xxxx, gst-plugins-*:shared=xxxx? Is it not enough glib:shared=True?

I mean, if glib is static, then everything needs to be static; but if it is shared there is no problem downstream.

maybe it can work, I didn't test that configuration(s). anyway, it's not something CCI ever builds.

jgsogo · 2021-10-18T09:03:32Z

Sure, but it will affect the validate() and enable more configurations. The check should be something like:

if self.options["glib"].static and self.options.shared:
    raise ConanInvalidConfiguration

and the same check should be done for all the consumers of glib... together with the self.info.requires["glib"].full_package_mode()

ericLemanissier · 2021-10-18T12:19:34Z

recipes/gstreamer/all/conanfile.py

@@ -114,6 +120,9 @@ def package(self):
        tools.rmdir(os.path.join(self.package_folder, "share"))
        tools.remove_files_by_mask(self.package_folder, "*.pdb")

+    def package_id(self):
+        self.info.requires["glib"].full_package_mode()


shouldn't we use shared_library_package_id here ?

that package id is when you are linking shared + static, here we are aligned (shared + shared; static + static)

comment from conan developers (conan-io/conan#9712):

The shared_library_package_id() helper was introduced to try to alleviate this issue, but it was not matured and it was not adopted in ConanCenter recipes.

I don't think it is worth the huge effort to improve and adopt the shared_library_package_id(), but if anything is to be done, it should be opt-in.

recipes/gstreamer/all/conanfile.py

SSE4 · 2021-10-26T15:12:19Z

Sure, but it will affect the validate() and enable more configurations. The check should be something like:
if self.options["glib"].static and self.options.shared:
    raise ConanInvalidConfiguration
and the same check should be done for all the consumers of glib... together with the self.info.requires["glib"].full_package_mode()

I have relaxed the check for now, as it requires more investigation (testing more different combinations of shared/static configurations for Glib/GStreamer/GstPlugins). I am marking PR as ready for review, let's move forward.

SSE4 · 2021-10-26T15:18:48Z

Therefore I advice to:

override default shared option of gstreamer to True if Windows (in config_options())

force glib shared in configure() if gstreamer shared.

I think right now we (in conan center) have a consensus to not use an option override for the downstream dependencies.
instead, we're rejecting invalid configurations in validate via ConanInvalidConfiguration.
why?
first off, if two or more consumers use an override to different values, it will result in a conflict.
second, it's surprising for the users to suddenly/silently get a different configuration from the one they requested.

there are different concerns on why shared/static builds might not be desired, e.g.:

license issues, e.g. some licenses like LGPL may require additional effort for static linking
distribution issues, you need an additional code to sign, obfustace or install all the shared objects
security issues, e.g. it might be harder to deploy hot fixes for statically linked objects, as it needs re-compilation
platform issues, like some platforms do not allow or support shared libraries (iOS, NaCl) or have significant reestrictions (Android)

so it might be a big pain if you suddenly find some unexpected shared/static library you didn't ask for

recipes/gstreamer/all/conanfile.py

conan-center-bot · 2021-10-28T12:58:59Z

All green in build 7 (57632df7868c8ed9b5ae5da2e06704b3ff64bcc8):

gstreamer/1.18.0@:
All packages built successfully! (All logs)
gstreamer/1.18.3@:
All packages built successfully! (All logs)
gstreamer/1.16.2@:
All packages built successfully! (All logs)
gstreamer/1.18.4@:
All packages built successfully! (All logs)
gstreamer/1.19.1@:
All packages built successfully! (All logs)
gstreamer/1.19.2@:
All packages built successfully! (All logs)

prince-chrismc

Providing bad binaries is worse then providing nothing at all imo

I am okay with this

* - [gstreamer] use full_package_mode for GLib Signed-off-by: SSE4 <[email protected]> * Update recipes/gstreamer/all/conanfile.py Co-authored-by: SpaceIm <[email protected]> * Update recipes/gstreamer/all/conanfile.py * Update recipes/gstreamer/all/conanfile.py * Update recipes/gstreamer/all/conanfile.py Co-authored-by: SpaceIm <[email protected]>

SSE4 requested review from jgsogo, uilianries and danimtb October 17, 2021 13:15

This was referenced Oct 17, 2021

[service] Potentially conflicting recipes #3571

Closed

- [gstreamer] bump glib to 2.70.0 #7634

Closed

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Oct 17, 2021

- [gstreamer] use full_package_mode for GLib

811b766

Signed-off-by: SSE4 <[email protected]>

SSE4 force-pushed the gstreamer_full_package_mode_glib branch from 4218442 to 811b766 Compare October 17, 2021 13:18

SpaceIm reviewed Oct 17, 2021

View reviewed changes

recipes/gstreamer/all/conanfile.py Outdated Show resolved Hide resolved

Update recipes/gstreamer/all/conanfile.py

8eb7c40

Co-authored-by: SpaceIm <[email protected]>

This comment has been minimized.

Sign in to view

conan-center-bot added Unexpected Error and removed Failed labels Oct 17, 2021

This comment has been minimized.

Sign in to view

conan-center-bot removed the Unexpected Error label Oct 17, 2021

ericLemanissier reviewed Oct 18, 2021

View reviewed changes

SSE4 commented Oct 26, 2021

View reviewed changes

recipes/gstreamer/all/conanfile.py Outdated Show resolved Hide resolved

SSE4 commented Oct 26, 2021

View reviewed changes

recipes/gstreamer/all/conanfile.py Outdated Show resolved Hide resolved

SSE4 added 2 commits October 26, 2021 08:09

Update recipes/gstreamer/all/conanfile.py

238930b

Update recipes/gstreamer/all/conanfile.py

ac4c3a8

SSE4 marked this pull request as ready for review October 26, 2021 15:12

SSE4 closed this Oct 27, 2021

SSE4 reopened this Oct 27, 2021

SSE4 closed this Oct 28, 2021

SSE4 reopened this Oct 28, 2021

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Oct 28, 2021

SSE4 commented Oct 28, 2021

View reviewed changes

recipes/gstreamer/all/conanfile.py Outdated Show resolved Hide resolved

Update recipes/gstreamer/all/conanfile.py

57632df

conan-center-bot removed the Failed label Oct 28, 2021

prince-chrismc mentioned this pull request Oct 28, 2021

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

conan-center-bot requested review from czoido and memsharded October 29, 2021 12:00

prince-chrismc approved these changes Nov 7, 2021

View reviewed changes

uilianries approved these changes Nov 8, 2021

View reviewed changes

madebr approved these changes Nov 20, 2021

View reviewed changes

conan-center-bot merged commit d2c793e into conan-io:master Nov 20, 2021

SSE4 mentioned this pull request Nov 22, 2021

- [gst-plugins-base] use full package mode #8148

Merged

4 tasks

SSE4 mentioned this pull request Jun 5, 2022

[package] glib: Can't have multiple dependencies linking against static glib in Windows #11022

Open

This was referenced May 18, 2023

glib: regenerate binaries #17607

Closed

glib: revert shared option to be False by default #17610

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

- [gstreamer] use full_package_mode for GLib #7734

- [gstreamer] use full_package_mode for GLib #7734

SSE4 commented Oct 17, 2021 •

edited

Loading

This comment has been minimized.

SpaceIm commented Oct 17, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

jgsogo commented Oct 18, 2021 •

edited

Loading

SSE4 commented Oct 18, 2021

jgsogo commented Oct 18, 2021

ericLemanissier Oct 18, 2021

uilianries Oct 18, 2021

SSE4 Oct 26, 2021

SSE4 commented Oct 26, 2021

SSE4 commented Oct 26, 2021

This comment has been minimized.

conan-center-bot commented Oct 28, 2021

prince-chrismc left a comment

- [gstreamer] use full_package_mode for GLib #7734

- [gstreamer] use full_package_mode for GLib #7734

Conversation

SSE4 commented Oct 17, 2021 • edited Loading

This comment has been minimized.

SpaceIm commented Oct 17, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

jgsogo commented Oct 18, 2021 • edited Loading

SSE4 commented Oct 18, 2021

jgsogo commented Oct 18, 2021

ericLemanissier Oct 18, 2021

Choose a reason for hiding this comment

uilianries Oct 18, 2021

Choose a reason for hiding this comment

SSE4 Oct 26, 2021

Choose a reason for hiding this comment

SSE4 commented Oct 26, 2021

SSE4 commented Oct 26, 2021

This comment has been minimized.

conan-center-bot commented Oct 28, 2021

prince-chrismc left a comment

Choose a reason for hiding this comment

SSE4 commented Oct 17, 2021 •

edited

Loading

SpaceIm commented Oct 17, 2021 •

edited

Loading

jgsogo commented Oct 18, 2021 •

edited

Loading