Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
- add gst-plugins-base 1.19.1 #7142
Changes from 9 commits
0621857
233f074
b2c9b77
b7e6387
73f8840
22d7beb
5ceacdb
5d2fa1d
d2b56e1
630ee2d
b2988a0
54f3d16
ee7480c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate already has this check.
see above on line 42: https://github.com/conan-io/conan-center-index/pull/7142/files#diff-65119968a00cace4db47857f3233cdca7f2de2e0f3ff99cbd57a8fae7a271682R42
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
and many errors of that type:
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:
what do you think happens?
I am requesting all
glib
,gstreamer
andgst-plugins-base
to be shared libraries.does conan guarantee that all of them will be shared?
nope, it surprisingly consumes
gstreamer:shared=True
linked toglib:shared=False
.and there seems to be no way to override it from command line?
why does it happen?
both packages
gstreamer:shared=True
withglib:shared=True
andgstreamer:shared=True
withglib:shared=False
have the same package id!for me, running on Mac:
I get the exactly same result:
so the options
glib:shared
is erased from the package id of thegstreamer
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.
There was a problem hiding this comment.
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
andfull_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 a
post_validate
method.A method in which a recipe can check the complete dependency tree.
Then , the following can be added to the
glib
recipe:There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)