-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[RFC] add prefer_static built-in option #9603
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9603 +/- ##
=======================================
Coverage 67.80% 67.80%
=======================================
Files 400 400
Lines 85522 85532 +10
Branches 18825 18827 +2
=======================================
+ Hits 57987 57997 +10
+ Misses 23035 23033 -2
- Partials 4500 4502 +2
Continue to review full report at Codecov.
|
e19f693
to
af20ebe
Compare
Fixed the tests. |
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.
Overall looks like a nice change to make.
While we are here, though, there are a couple things that should be cleaned up on general principle, and if you do clean them up, it means actually implementing this new feature changes fewer lines. With that in mind...
mesonbuild/dependencies/hdf5.py
Outdated
@@ -51,7 +51,8 @@ def __init__(self, name: str, environment: 'Environment', kwargs: T.Dict[str, T. | |||
newinc = [] # type: T.List[str] | |||
for arg in self.compile_args: | |||
if arg.startswith('-I'): | |||
stem = 'static' if kwargs.get('static', False) else 'shared' | |||
static_opt = environment.coredata.get_option(OptionKey('prefer_static')) | |||
stem = 'static' if kwargs.get('static', static_opt) else 'shared' |
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.
kwargs.get('static', False)
seems sub-optimal and as a general cleanup we can probably use self.static
here (as anywhere that inherits from ExternalDependency).
mesonbuild/dependencies/hdf5.py
Outdated
args = self.get_config_value(['-show', '-c'], 'args')[1:] | ||
args += self.get_config_value(['-show', '-noshlib' if kwargs.get('static', False) else '-shlib'], 'args')[1:] | ||
args += self.get_config_value(['-show', '-noshlib' if kwargs.get('static', static_opt) else '-shlib'], 'args')[1:] |
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.
Likewise here.
mesonbuild/dependencies/misc.py
Outdated
@@ -175,7 +175,7 @@ def __init__(self, name: str, environment: 'Environment', kwargs: T.Dict[str, T. | |||
return | |||
|
|||
self.name = 'python3' | |||
self.static = kwargs.get('static', False) | |||
self.static = kwargs.get('static', environment.coredata.get_option(mesonlib.OptionKey('prefer_static'))) |
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.
This seems odd! Why does this line exist, it seems to be literally overriding the existing self.static attribute...
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.
Do you mean that this is attribute is already inherited from base.py
? Yeah, I can just delete the line in that case.
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.
That is exactly what I mean. :)
@@ -537,7 +537,7 @@ def shaderc_factory(env: 'Environment', | |||
shared_libs = ['shaderc'] | |||
static_libs = ['shaderc_combined', 'shaderc_static'] | |||
|
|||
if kwargs.get('static', False): | |||
if kwargs.get('static', env.coredata.get_option(mesonlib.OptionKey('prefer_static'))): |
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.
This, however, is a function, not a class, so kwargs.get() is correct as is your modification.
af20ebe
to
92dd836
Compare
static = Mesonlib.LibType.STATIC if kwargs.get('static', False) else Mesonlib.LibType.SHARED | ||
get_option = environment.coredata.get_option | ||
static_opt = kwargs.get('static', get_option(Mesonlib.OptionKey('prefer_static')) | ||
static = Mesonlib.LibType.STATIC if static_opt else Mesonlib.LibType.SHARED |
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 change still be here? All of this is comments so I guess it doesn't matter too much, but I don't want to add wrong things in here.
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.
Yeah, this documentation is, in part, showing how it all fits together. So I believe it is correct to demonstrate the underlying implementation. IIRC a major motivation in adding this comment section was so that we could move from one core dev understanding how the mesonbuild/dependencies/
directory works, to multiple core devs understanding.
After all, there is also a self.libtype
which can technically be used too, but that doesn't obsolete the comment tutorial either.
This pull request introduces 1 alert when merging 92dd836 into e77582b - view on LGTM.com new alerts:
|
92dd836
to
85e11b5
Compare
Split this into two commits now and rebased. |
I haven't looked at code, does it cover both dependency() and find_library()? This new option is better than nothing but unfortunately it's a all-or-nothing option. Ideally we should have more fine grained way of telling which deps should be static and which should be shared. A long time ago I made that attempt: #4276, and a few others, but we never agreed on a good solution. The use-case I still have is when you build an app (e.g. GStreamer) for a platform that provide shared libs for some basic deps (e.g. zlib). In that case I want to use the shared libs for stuff provided by the device's base platform and static link everything else into my app. |
That said, since we had that use-case for years and we never found an elegant enough solution, I wouldn't block this PR that already covers a good part of the use-case. |
That's a really good point. This does not cover find_library() and it really should. I'll fix that. |
8392fe9
to
b183005
Compare
I added support to |
In a couple of spots, kwargs.get('static', False) was being unneccesarily used. In these spots, we can just use self.static instead which is already inherited from the ExternalDependency. In additional, the python system dependency oddly has a kwargs.get('static', False) line which overrides the self.static in that dependency for no real reason. Delete this line too.
By default, meson will try to look for shared libraries first before static ones. In the meson.build itself, one can use the static keyword to control if a static library will be tried first but there's no simple way for an end user performing a build to switch back and forth at will. Let's cover this usecase by adding an option that allows a user to specify if they want dependency lookups to try static or shared libraries first. The writer of the meson.build can manually specify the static keyword where appropriate which will override the value of this option.
b183005
to
2a12518
Compare
This would be an incredibly useful to have feature for VLC's WIP meson port, currently we clutter every dependency and find_library with the static kwarg to get this ability. |
Throwing just an idea: what I often want is use shared libraries from the system, and if not found build a static fallback subproject. Not sure how we could try to express that use-case, |
Perhaps the prefer_static option should cause all subprojects to default to static lookup? IIRC we already special case |
Yes we do that, so if I understand correctly prefer_static as proposed would already build static subprojects. However my use-case is a little bit different: I build GStreamer against a platform SDK that contains a few basic libraries (e.g. zlib, openssl) and I want to use the shared libraries provided by the platform for them. But everything else that is not part of the SDK is built as subproject and those would be nice to be static. So |
Well this PR currently only touches code related to
If such an option is added, would it make sense to turn the |
@xclaesse: Just revisiting this one, but if I understand correctly, would your usecase be covered if As a sidenote, I finally got around to confirming that yes, subprojects do inherit the |
Doesn't |
Yes, but it means editing entry dependency() calls. I would like instead a global switch for that. Preferring static or shared is often a user choice rather than a project choice. |
Re-thinking about this, subprojects inherit Ultimately, we could add a list of libs/deps in native/cross file that tells exactly which one should be static. |
Anything else that this PR needs? |
No, I don't think so. And it seems we're all in agreement that this option is useful as it stands. |
Thanks! |
Could this be added to the documentation? The reference manual doesn't mention Ref #3084 |
It's a built-in option. You can see it here. https://mesonbuild.com/Builtin-options.html#core-options |
I have no objection to also adding a reference to the builtin option, in the documentation for @sertonix would you like to submit a patch for this? |
By default, meson will try to look for shared libraries first before
static ones. In the meson.build itself, one can use the static keyword
to control if a static library will be tried first but there's no simple
way for an end user performing a build to switch back and forth at will.
Let's cover this usecase by adding an option that allows a user to
specify if they want dependency lookups to try static or shared
libraries first. The writer of the meson.build can manually specify the
static keyword where appropriate which will override the value of this
option.
I suppose this needs test cases to go with it? I was a bit lost on where exactly tests should be made/created so that was left out for now (guidance would be appreciated).
Fixes #2816
Fixes #4276
Fixes #6109
Fixes #6629
Fixes #7621