fix regression in converting build_target kwargs to typed_kwargs #12505
Conversation
We haven't actually verified that these kwargs are equal to what we had
before, and should probably revert the entire series. But I have
multiple reports in the wild of projects that no longer build because of
`install: [true, false, get_option('foobar')]` which was always
incorrect and always equal to just dropping values all over the floor
and treating it the same as "bool(value) == True".
Special case this particular typed kwarg and allow it with a sternly
worded warning that it was always wrong and should never ever ever be
done.
Fixes: https://bugs.gentoo.org/917118
Fixes: http://qa-logs.debian.net/2023/11/11/rhythmbox_3.4.7-1_unstable_meson-exp.log
Thanks to the Gentoo Tinderbox project, and Lucas Nussbaum of the Debian
project.
|
I'm concerned that there's going to turn out to be a bunch more regressions and we are going to regret everything. |
| @@ -573,7 +573,8 @@ def _objects_validator(vals: T.List[ObjectTypes]) -> T.Optional[str]: | |||
| OVERRIDE_OPTIONS_KW, | |||
| KwargInfo('build_by_default', bool, default=True, since='0.38.0'), | |||
| KwargInfo('extra_files', ContainerTypeInfo(list, (str, File)), default=[], listify=True), | |||
| INSTALL_KW, | |||
| # Accursed. We allow this for backwards compat and warn in the interpreter. | |||
| KwargInfo('install', object, default=False), | |||
There was a problem hiding this comment.
I think INSTALL_KW = KwargInfo('install', bool, default=False) is generally wrong. We always allowed that kwarg to be a list of boolean for the case it has multiple outputs. That's especially useful for CustomTarget, but also for any BuildTarget that could have extra vala outputs IIRC.
There was a problem hiding this comment.
Oh wait, maybe that was only for install_dir?
There was a problem hiding this comment.
Right, it is install_dir that allows a list, maybe the warning should hint at doing that, because it very likely was the intention when doing install: [true, false, get_option('foobar')].
I'm wondering why this suddenly happens, the PR that changed this is 2 years old.
There was a problem hiding this comment.
The PR that what'd this? We do no form of type checking on this kwarg at all. That's why I've written object here. It's entirely possible for people to have been using this kwarg with install: executable('foo', 'foo.c') which is total nonsense but also a valid object.
One of the reports I got was for someone confusing it for install_dir (despite the fact that they use install_dir correctly below). The other report I got was someone doing install: 'true' which makes me wonder if people are also doing install': 'false' and getting... bool('false') == True. Perhaps they were more used to JavaScript....
We haven't actually verified that these kwargs are equal to what we had before, and should probably revert the entire series. But I have multiple reports in the wild of projects that no longer build because of
install: [true, false, get_option('foobar')]which was always incorrect and always equal to just dropping values all over the floor and treating it the same as "bool(value) == True".Special case this particular typed kwarg and allow it with a sternly worded warning that it was always wrong and should never ever ever be done.
Fixes: https://bugs.gentoo.org/917118
Fixes: http://qa-logs.debian.net/2023/11/11/rhythmbox_3.4.7-1_unstable_meson-exp.log
Thanks to the Gentoo Tinderbox project, and Lucas Nussbaum of the Debian project.