-
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
Add <lang>_(static|shared)_args #11981
Add <lang>_(static|shared)_args #11981
Conversation
Fair enough! This seems like a more than adequate approach. |
d69e21f
to
cfdbb44
Compare
This goes full circle, I made a similar PR 5 years ago... IIRC one reason for not going that path is because there are other kwargs that needs to be different for static/shared, like From implementation POV, I think it's missing setting |
@@ -44,3 +44,8 @@ kwargs: | |||
description: | | |||
Specify a Microsoft module definition file for controlling symbol exports, | |||
etc., on platforms where that is possible (e.g. Windows). | |||
|
|||
<lang>_shared_args: | |||
type: str | file |
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.
Why file
?
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.
Checking how <lang>_args
works, some parts of the code seems to allow File, but documentation does not mention that, and I see many code paths that does not seems to support it at first glance.
Allowing File does make sense, I guess it's common to pass a file in compiler arguments, but doc needs to be updated and it would have to be catched as new feature by KwargInfo. It would also need to ensure we have a unit test for that, because from reading the code I'm not convinced it works. Probably we need a convertor in KwargInfo to only get strings?
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.
Passing arguments as files is a niche use case because the main time you do that is for sources (which of course supports files() objects). There are some cases where you want to pass non-source files, but the main example I can think of is linker scripts and particularly -Wl,--version-script=<file>
which simply doesn't work as a discrete file object.
You could pass ['-T', files('foobar.ld')]
though, if you're doing firmware rather than boring old "shared libraries with versioned symbols".
For compile time options there's ['-include', files('sharedmacros.h')]
but use of '-include'
is typically done at the project scope.
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 guess that should actually be list[str | file]
. And that the documentation for <lang>_args
is wrong too, since it says list[str]
, but should also be list[str | file]
I fixed both in the latest version
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 would leave this out of this PR because I'm not convinced it actually works, or add a unit test with a File in c_args to convince me...
If I understand correctly, we use that there: https://github.com/mesonbuild/meson/blob/master/mesonbuild/backend/ninjabackend.py#L2923, and will crash there: https://github.com/mesonbuild/meson/blob/master/mesonbuild/backend/backends.py#L959. But I may be missing something in how that code works.
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.
ahah, yes, as predicted:
File "/home/xclaesse/programmation/meson/mesonbuild/backend/ninjabackend.py", line 2923, in _generate_single_compile_target_args
commands += self.escape_extra_args(target.get_extra_args(compiler.get_language()))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/xclaesse/programmation/meson/mesonbuild/backend/backends.py", line 959, in escape_extra_args
if arg.startswith(('-D', '/D')):
^^^^^^^^^^^^^^
AttributeError: 'File' object has no attribute 'startswith'
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.
So this is extra awesome, because this totally works for Vala. So it has to be handled here, since we have to allow it or we'd regress working code...
There is even a manual tests/7 vala composite widgets
that tests this, though it doesn't seem we have an automated test that does. So we can't take this out, it has to remain. Sigh
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.
But maybe we can handle this in the interpreter instead of sending it to the backend at all....
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.
Okay. So I did some git archeology. This was explicitly turned on for Vala in 0.25 (see: cd40187). That means that turning it off would constitute a regression in my mind, since the explicit purpose of the patch was to allow this. So, I've allowed this for Vala, and only for Vala. And I've moved the conversion code to the Interpreter level, so that the backend/middle layer doesn't have to be aware of this (except for creating the dependency)
046b1b9
to
eee754c
Compare
f6dcd7e
to
7413478
Compare
7413478
to
9e0bd72
Compare
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.
LGTM. I really need this feature.
933f665
to
1cc66c0
Compare
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.
s/both_library/both_libraries/g
?
_EXCLUSIVE_STATIC_LIB_KWS: T.List[KwargInfo] = [ | ||
*[l.evolve(name=f'{l.name.split("_", 1)[0]}_static_args', since='1.3.0') | ||
for l in _LANGUAGE_KWS], |
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.
Why are we adding <lang>_<static/shared>_args
to the static/shared library functions as well? Shouldn't this just be for the target types that are able to support both and have to differentiate?
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 didn't see any reason not to add it here, the implementation has to handle it either way (and it's simpler from an implementation point of view this way), but I can drop it from shared_library
and static_library
if you'd prefer.
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 didn't see any reason not to add it here, the implementation has to handle it either way (and it's simpler from an implementation point of view this way), but I can drop it from shared_library and static_library if you'd prefer.
1cc66c0
to
c7c0ea5
Compare
24d0288
to
58881ce
Compare
14b39de
to
0eed088
Compare
@dcbaker I think so, yes. If it's easier for the implementation to know about it I don't mind, but I'd like to discourage people from going out of their way to define a static_library and then passing Unless someone disagrees with me and would like to argue in favor of it being a UX enhancement, in which case I'd love to hear those arguments (or even concede the point as "I don't feel strongly enough against it as long as someone else feels strongly in favor".) I would however like to avoid the situation where UX features are added based on the rationale that the implementation is simpler, rather than because it's a desired feature. There are a bunch of reasons why I feel this way, but maybe it will be convincing if I point out that that's how we ended up with build_target in its current state, complete with now-deprecated java support. |
0eed088
to
79d46a9
Compare
@eli-schwartz done. Now allowed only for library, both_library, and build_target |
That is fixed as well |
79d46a9
to
ef0580c
Compare
Docs issue fixed locally |
c6fa0d1
to
faaef61
Compare
faaef61
to
f404289
Compare
_BASE_LANG_KW: KwargInfo[T.List[str]] = KwargInfo( | ||
'UNKNOWN', | ||
ContainerTypeInfo(list, (str)), | ||
listify=True, | ||
default=[], | ||
) |
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 feel like this belongs in commit 2, since even there we are producing this in a list comprehension plus repeating it for rust_args separately.
But also, the per-commit history shows an oddity, you add the initial version for most languages with File support and then remove it when migrating to the shared _BASE_LANG_KW
.
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 feel like that got squashed into the wrong place... sigh
5b4451c
to
a3ecfb3
Compare
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 is fantastic stuff.
@@ -724,7 +724,7 @@ def __init__( | |||
objects: T.List[ObjectTypes], | |||
environment: environment.Environment, | |||
compilers: T.Dict[str, 'Compiler'], | |||
kwargs): | |||
kwargs: T.Dict[str, T.Any]): |
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.
Couldn't we use TYPE_kwargs 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.
I don't think that's accurate though, since TYPE_kwargs
includes several Interpreter
types in its values
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.
Well, it is narrower than Any which explicitly allows anything. However that may not be too big of a deal... I guess it is easier to grep later on than incorrect use of TYPE_kwargs... and it allows checking the keys of course.
Way back in Meson 0.25, support was added to `vala_args` for Files. Strangely, this was never added to any other language, though it's been discussed before. For type safety, it makes more sense to handle this in the interpreter level, and pass only strings into the build IR. This is accomplished by adding a `depend_files` field to the `BuildTarget` class (which is not exposed to the user), and adding the depend files into that field, while converting the arguments to relative string paths. This ensures both the proper build dependencies happen, as well as that the arguments are always strings.
This also moves the repacking into the interpreter, making the build implementation simpler and removing a layering violation. This also makes use a defaultdict to remove the need to call `.get()`
`java_args` is only valid for `jar()` (and `build_target()`, but that's deprecated), while all other language args are invalid for `jar()`. This deprecates all of those arguments, that shouldn't be allowed, and provides useful error messages. As an advantage, this avoids generating useless `java_static_args` and `java_shared_args`. In order to get useful error messages for both build_target and executable + *library, we need to separate LIBRARY and BUILD_TARGET a bit.
This allows for even more accurate type information
a3ecfb3
to
3374a85
Compare
Which allow passing arguments specifically to the static or shared libraries. For design, this is all handled in the interpreter, by the build layer the arguments are combined into the existing fields. This limits changes required in the mid and backend layers
3374a85
to
2412dfb
Compare
These options should have corresponding options in |
@bb010g I'm workingo n that. Unfortunately declare_dependecy() doesn't currently have the concept of per-language arguments, and needs additional work to get there before we can add these. |
@dcbaker Are we likely to see |
This adds an option to pass
c_static_args
andc_shared_args
. I thought about the two basic approaches we've discussed, which is adding a dictionary argument type toc_args
, or adding additional keyword arguments.I went with the additional keyword arguments for a couple of reasons:
build_target()
orlibrary()
orboth_library()
or a specific target, so what is passed the build layer will still need to beMapping[str, str | File]
When I implemented the dictionary approach, even more code was required, and tracking the correct since values turned out to be much harder, plus there would be no good way to add new since values to the dictionary itself, our current infrastructure doesn't support that and would have to be extended.