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.

Sign up for GitHub

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

Jump to bottom

Fix regression when using both libraries #13835

Closed
wants to merge 1 commit into from
Closed

Fix regression when using both libraries #13835

wants to merge 1 commit into from

Conversation

xclaesse
Copy link
Member

@xclaesse xclaesse commented Oct 29, 2024 •
edited
Loading 

This partly reverts #12632 and provide an alternative implementation.
The issue is we cannot propagate BothLibraries objects beyond the
interpreter because it breaks many isinstance(obj, SharedLibrary) cases.

Instead make SharedLibrary and StaticLibrary cross reference each other
so we can take their brother library in as_static() and as_shared()
methods.

@xclaesse xclaesse requested a review from jpakkane as a code owner October 29, 2024 12:46
@xclaesse
Copy link
Member Author

xclaesse commented Oct 29, 2024 •
edited
Loading

This test pass with Meson 1.5, fails since 1.6 but pass when using link_with: libfoo.get_shared_lib().

git bisect pointed to #12632.

CC @tp-m @bruchar1

@xclaesse xclaesse mentioned this pull request Oct 29, 2024
Merged
@bruchar1
Copy link
Member

@xclaesse I made a fix in #13837, including this test. Let me know if it solves the problem.

@xclaesse xclaesse changed the title Add regression test for GStreamer introspection issue Fix regression when using both libraries Oct 31, 2024
@xclaesse xclaesse mentioned this pull request Oct 31, 2024
Merged
@dcbaker
Copy link
Member

dcbaker commented Oct 31, 2024

What happens if you have an explicitly static or shared use and you call as_<the other one>() with this patch?

It seems like I have something like:

x = declare_dependency(link_with : both_libraries(...).as_shared())
y = x.as_static()

y will have the library from x as static, even though I've explicitly declared that it's shared

@xclaesse
Copy link
Member Author

xclaesse commented Nov 1, 2024

y will have the library from x as static, even though I've explicitly declared that it's shared

That's true. What's the desired behavior here is a bit debatable, but I think in any case that's a small price to pay compared to having to deal with BothLilbraries objects everywhere across the whole meson code base. I don't have an immediate idea to tackle this niche case.

@xclaesse
Copy link
Member Author

xclaesse commented Nov 1, 2024

@dcbaker note the alternative is #13837, but my fear is we access InternalDependency.libraries in many places and now they can contain BothLibraries objects which might not be expected. It seems impossible to know every places that needs that fix...

@xclaesse
Fix regression when using both libraries
36cd1ff 
This partly reverts mesonbuild#12632 and provide an alternative implementation.
The issue is we cannot propagate BothLibraries objects beyond the
interpreter because it breaks many isinstance(obj, SharedLibrary) cases.

Instead make SharedLibrary and StaticLibrary cross reference each other
so we can take their brother library in as_static() and as_shared()
methods.
xclaesse
xclaesse commented Nov 1, 2024
View reviewed changes
mesonbuild/build.py
def extract_targets_as_list(self, kwargs: T.Dict[str, T.Union[LibTypes, T.Sequence[LibTypes]]], key: T.Literal['link_with', 'link_whole']) -> T.List[LibTypes]:
bl_type = self.environment.coredata.get_option(OptionKey('default_both_libraries'))
if bl_type == 'auto':
bl_type = 'static' if isinstance(self, StaticLibrary) else 'shared'
Copy link
Member Author

Choose a reason for hiding this comment

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

actually, reading this again, my PR is wrong, it's not handling the "auto" case anymore. :/

Copy link
Member

Choose a reason for hiding this comment

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

I fixed it in #13837

bruchar1
bruchar1 reviewed Nov 6, 2024
View reviewed changes
mesonbuild/modules/pkgconfig.py
@@ -210,7 +210,7 @@ def _process_libs(
if obj.found():
processed_libs += obj.get_link_args()
processed_cflags += obj.get_compile_args()
elif isinstance(obj, build.SharedLibrary) and obj.shared_library_only:
elif isinstance(obj, build.SharedLibrary) and obj.static_library is None:
Copy link
Member

Choose a reason for hiding this comment

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

This change brokes the test_as_link_whole unit test

@eli-schwartz
Copy link
Member

@xclaesse are we using this PR or the other PR? What's the story with getting this issue fixed?

@xclaesse
Copy link
Member Author

xclaesse commented Dec 1, 2024

This PR is not ready and I don't have much time to polish it. I would say in the meantime the other fix could be good to have in 1.6.x because it fix at least a pretty important use-case.

@bruchar1
Copy link
Member

bruchar1 commented Dec 2, 2024

This PR is not ready and I don't have much time to polish it. I would say in the meantime the other fix could be good to have in 1.6.x because it fix at least a pretty important use-case.

@xclaesse I think you missed the point that I rewrote #13837 based on what you tried to do here. It is no longer just a patch to fix the gnome module.

@bruchar1
Copy link
Member

bruchar1 commented Dec 6, 2024

#13837 was merged

@bruchar1 bruchar1 closed this Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bruchar1 bruchar1 bruchar1 left review comments

@eli-schwartz eli-schwartz eli-schwartz requested changes

@jpakkane jpakkane Awaiting requested review from jpakkane jpakkane is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xclaesse @bruchar1 @dcbaker @eli-schwartz