Use typed_kwargs with `dependency()` #13995

dcbaker · 2024-12-09T23:04:53Z

This only converts the keyword arguments that are defined in the interpreter, which means that many arguments that are specific to certain dependencies are not covered here, and continue to rely on being handled further down the stack. That is a problem I'd like to address, but this is already 31 commits, and I don't want to make it even harder to review. This also attempts to minimize changes below the interpreter (with the exception of updating the dependency key generating function), again, for the purposes of making review easier and not creating too large of a change all at once. This means in some cases we now awkwardly handle the type we really want to see, as well as the raw type from the DSL that we don't want in the lower levels (bool vs MachineChoice as pretty obvious example).

I have made an effort to make this easier to review and bisect by having numerous small patches. This should also make it possible to merge this series in pieces if that's desirable, because I've tried very hard to make everything work at all points in the series.

mesonbuild/interpreter/dependencyfallbacks.py

mesonbuild/interpreter/interpreter.py

dcbaker · 2024-12-09T23:09:33Z

I noticed that some of the linters are failing, and I have fixups for those locally, but to avoid spamming CI I'll wait to push those until after I've seen what tests fail in CI that passed locally.

mesonbuild/dependencies/cmake.py

dnicolodi · 2024-12-10T11:01:52Z

mesonbuild/dependencies/detect.py

+        'default_options', 'not_found_message', 'include_type'}
+    valid_keys = [n for n in DEPENDENCY_KWS
+                  if n.name not in invalid_keys]
+    for dep in valid_keys:


Why don't you like the continue? In this form, the list of keys needs to be iterated twice, which seems wasteful.

I would have written:

for dep in DEPENDECY_KWS: if dep.name in invalid_keys: continue ...

Because this simplifies the loop body, and I generally prefer that. I don't know that it really matters either way though?

No, it does not matter. I was just curious because you mentioned the elimination of the continue in the commit message as it was something important. I prefer the code as I wrote it before, but it is mostly personal preference, I guess.

Ah, that comment is a little out of date, and the continue I was calling attention to isn't removed (or added) anymore! Originally in this commit I changed the function to always include all of the relevant keys, but that broke previous commits, so this commit got split from one commit at the end of the series to two, one at the end and one at the beginning.

run_project_tests.py

bruchar1 · 2024-12-11T19:54:37Z

mesonbuild/dependencies/detect.py

@@ -62,7 +62,7 @@ def get_dep_identifier(name: str, kwargs: T.Dict[str, T.Any]) -> 'TV_DepID':
                assert isinstance(i, str), i
            value = tuple(frozenset(listify(value)))
        else:
-            assert isinstance(value, (str, bool, int)), value
+            assert value is None or isinstance(value, (str, bool, int)), value


isinstance(value, (types.NoneType, str, bool, int)), value ?

This is the idiomatic way to do it, though I do like the less typing of the single isinstance check. I wonder if anyone else has opinions?

mesonbuild/modules/python.py

eli-schwartz · 2024-12-12T18:04:57Z

mesonbuild/dependencies/base.py

+        if not isinstance(static, bool):
            raise DependencyException('Static keyword must be boolean')


How is it possible for it to be something other than (NoneType, bool) now that we typecheck this?

eli-schwartz · 2024-12-12T18:12:53Z

mesonbuild/interpreter/type_checking.py

@@ -857,4 +857,5 @@ def _pkgconfig_define_convertor(x: T.List[str]) -> PkgConfigDefineType:
    DEFAULT_OPTIONS.evolve(since='0.38.0'),
    KwargInfo('allow_fallback', (bool, NoneType), since='0.56.0'),
    KwargInfo('cmake_args', ContainerTypeInfo(list, str), listify=True, default=[], since='0.50.0'),
+    KwargInfo('cmake_module_path', ContainerTypeInfo(list, str), listify=True, default=[], since='0.50.0'),


Surely we should be e.g. removing mesonbuild/dependencies/cmake.py:

cm_path = stringlistify(extract_as_list(kwargs, 'cmake_module_path'))

and just using kwargs.get('cmake_module_path', []) (assuming we can construct it from places other than func_dependency())

eli-schwartz

Not going to do a full review, a cursory inspection reveals this PR has the same problems that plagued the previous big typed_kwargs rewrite: general failure to demonstrate that the typed_kwargs are equal to what we had before by cross-comparing to our existing handrolled build layer checks and removing the obsolete bits.

Trying to review that PR just got me ignored, so I have no motivation to review this one.

dcbaker · 2024-12-12T20:07:10Z

@eli-schwartz I'm not really sure what the way forward is then. The last time I made a PR like this I did makes changes to the mid and lower lyayrs to make sure everything worked correctly, it was hundreds of patches, and each one was huge. So that went around and around in review, having to be rebased several times, often leading to days of work trying to get everything correctly again after the rebase, only to have to repeat it again a week later. That was super frustrating and de-motivating.

I tried this time to do the opposite, very small self contained changes that were conservative by leaving the lower level valadation in place until we could move to mypy based checking for those and remove the run time validation since we had static analysis to help ensure that the lower levels weren't getting invalid information.

What approach would make this better to review from your point of view?

Because in at least one case it will lack the `log_tried` method.

dcbaker · 2024-12-13T22:23:03Z

@eli-schwartz is this the sort of work you'd like to see? (I'm aware the names of some of the commits are not suitable for merging)

mesonbuild/dependencies/base.py

-
-class ExternalDependency(Dependency, HasNativeKwarg):
-    def __init__(self, type_name: DependencyTypeName, environment: 'Environment', kwargs: T.Dict[str, T.Any], language: T.Optional[str] = None):
+class ExternalDependency(Dependency):


mesonbuild/dependencies/cmake.py

@@ -17,6 +17,7 @@
 import typing as T

 if T.TYPE_CHECKING:
+    from .base import DependencyKWs


mesonbuild/dependencies/configtool.py

 if T.TYPE_CHECKING:
+    from .base import DependencyKWs


mesonbuild/dependencies/detect.py

@@ -6,16 +6,17 @@
 import collections, functools, importlib
 import typing as T

-from .base import ExternalDependency, DependencyException, DependencyMethods, NotFoundDependency
+from .base import ExternalDependency, DependencyException, NotFoundDependency


mesonbuild/dependencies/detect.py

@@ -6,16 +6,17 @@
 import collections, functools, importlib
 import typing as T

-from .base import ExternalDependency, DependencyException, DependencyMethods, NotFoundDependency
+from .base import ExternalDependency, DependencyException, NotFoundDependency


mesonbuild/dependencies/framework.py

 import typing as T

 if T.TYPE_CHECKING:
+    from .base import DependencyKWs


mesonbuild/dependencies/pkgconfig.py

@@ -19,6 +19,7 @@
 import typing as T

 if T.TYPE_CHECKING:
+    from .base import DependencyKWs


mesonbuild/interpreter/interpreterobjects.py

@@ -29,6 +29,8 @@
 import typing as T

 if T.TYPE_CHECKING:
+    from typing_extensions import Literal


mesonbuild/interpreter/mesonmain.py

@@ -25,6 +25,7 @@
 if T.TYPE_CHECKING:
    from typing_extensions import Literal, TypedDict

+    from ..dependencies.base import DependencyKWs


mesonbuild/interpreter/mesonmain.py

@@ -25,6 +25,7 @@
 if T.TYPE_CHECKING:
    from typing_extensions import Literal, TypedDict

+    from ..dependencies.base import DependencyKWs


dnicolodi · 2024-12-13T23:56:34Z

mesonbuild/dependencies/misc.py

-    language = kwargs.get('language', 'c')
-    if language not in ('c', 'cpp', 'fortran'):
+    language = kwargs.get('language')
+    if language not in (None, 'c', 'cpp', 'fortran'):


This commit and the next commit use two different ways of dealing with the same issue. I prefer the approach of the next commit where None is explicitly translated to 'c'. It may be that you don't do that here because because the value is sent down the call stack. However translating None to 'c' here is a first small step to reduce the permeation of this weird convention.

Sure, I've changed them both to the if language is None: language = 'c' approach

dnicolodi · 2024-12-13T23:58:25Z

mesonbuild/dependencies/dub.py

@@ -328,7 +328,7 @@ def find_package_target(pkg: DubPackDesc) -> bool:
        for lib in bs['libs']:
            if os.name != 'nt':
                # trying to add system libraries by pkg-config
-                pkgdep = PkgConfigDependency(lib, environment, {'required': 'true', 'silent': 'true'})
+                pkgdep = PkgConfigDependency(lib, environment, {'required': 'true', 'silent': True})


From the use of the plural form in the commit message I have the impression that both of these were meant to be True and not 'true'

They are both supposed to be true... something happened when I rebased this

For whatever reason Meson has always used None == <C Language>. This doesn't make a lot of sense to me, but it's how things currently work, and netcdf should handle that correctly.

I have not changed the underlying implementation, as there are more paths to get there than there are in CMake, and I don't want to spend hours fighting with all of that right now.

Static must be optional, because although the default is static=false, we special case that static : false is different than static being not set in a number of cases.

Since it's basically unusued, but the DEPENDENCY_KWS can be used instead

This gives us one source of truth, as well as ensure that an empty value and a default value are treated the same.

There are a couple of annoyances around the way Python's TypedDicts work that we should get out of the way: 1) A TypedDict keeps it's `total` status, so you can't do something like: ```python from interpreter.type_checking import FuncDependency class DependencyKWs(FuncDependency, total=False): pass ``` All of the arguments from FuncDependency remain required. 2) The "functional" form of `TypedDict` requires a literal for it's arguments, so you can't do something like: ```python _VALUES = {...} FuncDependency = TypedDict('FuncDependency', _VALUES) DependencyKWs = TypedDict('DependencyKWs', _VALUES, total=False) And, there are some arguments `dependency()` that are not passed to the lower levels, but are interpreter constructs. Likewise, some of the Dependency classes take additional kwargs that are not part of the dependency() DSL API. So we have to have two different TypedDicts, which is unfortunate. Additionally, TypedDicts must define all allowed keys, there is no way to define "extra keys". This means that implementing this change as a series of small commits would would require hundreds of `# type: ignore` comments to be introduced and removed across the range. That kind of noise seems counterproductive to the purpose of having small commits, since there will end up being loads of churn introduced by such a series of commits.

dnicolodi · 2024-12-14T00:27:51Z

mesonbuild/dependencies/base.py

-            static = self.env.coredata.get_option(OptionKey('prefer_static'))
-        self.static = T.cast('bool', static)
+            static = T.cast('bool', self.env.coredata.get_option(OptionKey('prefer_static')))
+        self.static = static


It does not matter much, but unless there is something subtle going on that I don't see, this hunk seem to simply change on which line the cast is done and could be either squashed with a patch a few commits down the stack, or simply suppressed.

Right, so mypy complains it's a useless cast because if static comes from kwargs, it's bool, and that's what we cast to. But if it comes from options then it's int | bool | str | list[str], so we still have to cast to not have a mypy error about assigning a different type. I honestly feel like that's a mypy bug, it really should be complaining that static is changing types if you don't cast.

dcbaker requested a review from jpakkane as a code owner December 9, 2024 23:04

github-advanced-security bot found potential problems Dec 9, 2024

View reviewed changes

mesonbuild/interpreter/dependencyfallbacks.py Dismissed Show dismissed Hide dismissed

mesonbuild/interpreter/interpreter.py Dismissed Show dismissed Hide dismissed

dcbaker force-pushed the submit/use-typed-kwargs-for-dependency branch 2 times, most recently from 11f34e7 to 2914829 Compare December 9, 2024 23:32

dnicolodi reviewed Dec 10, 2024

View reviewed changes

mesonbuild/dependencies/cmake.py Outdated Show resolved Hide resolved

dnicolodi reviewed Dec 10, 2024

View reviewed changes

dcbaker force-pushed the submit/use-typed-kwargs-for-dependency branch 5 times, most recently from 79d0290 to 0d7dee8 Compare December 10, 2024 19:26

bruchar1 reviewed Dec 11, 2024

View reviewed changes

dcbaker force-pushed the submit/use-typed-kwargs-for-dependency branch 2 times, most recently from 086e36d to 2f556cc Compare December 11, 2024 20:46

eli-schwartz reviewed Dec 12, 2024

View reviewed changes

eli-schwartz requested changes Dec 12, 2024

View reviewed changes

dcbaker added 2 commits December 13, 2024 09:46

dependencies/detect: make assertions more useful

31353dc

modules/python: Make sure that all dependency candidates meet interface

9df7f5f

Because in at least one case it will lack the `log_tried` method.

dcbaker force-pushed the submit/use-typed-kwargs-for-dependency branch from 2f556cc to a9d130b Compare December 13, 2024 22:22

github-advanced-security bot found potential problems Dec 13, 2024

View reviewed changes

dcbaker force-pushed the submit/use-typed-kwargs-for-dependency branch 3 times, most recently from 0ec8cea to 1f622f6 Compare December 13, 2024 23:35

dnicolodi reviewed Dec 13, 2024

View reviewed changes

dependencies/netcdf: correctly handle a language of None

bdd24d8

For whatever reason Meson has always used None == <C Language>. This doesn't make a lot of sense to me, but it's how things currently work, and netcdf should handle that correctly.

dcbaker added 28 commits December 13, 2024 16:19

dependencies: make components type safe

dc58b11

interpreter: port dependency fallback to typed_kwargs

9dc1755

interpreter: port dependency include_type to typed_kargs

a1cffdc

I have not changed the underlying implementation, as there are more paths to get there than there are in CMake, and I don't want to spend hours fighting with all of that right now.

dependencies: make include_type type safe

fbaad3a

interpreter: port dependency language to typed_kwargs

c129e3f

interpreter: port dependency main to typed_kwargs

dfdc732

interpreter: port dependency method to typed_kwargs

0699e84

dependencies: make method type safe

59874a8

interpreter: port dependency modules to typed_kwargs

8b08c6f

dependencies: make modules type safe

cd3e8cc

interpreter: port dependency native to typed_kwargs

75ea399

depenencies: make native/for_machine type safe

7504ecb

interpreter: port dependency not_found_message to typed_kwargs

c2592d1

interpreter: port dependency optional_modules to typed_kwargs

7a9b4c6

dependencies: make optional_modules type safe

8914dec

interpreter: port dependency private_headers to typed_kwargs

0d3428f

interpreter: port dependency required to typed_kwargs

e156fc6

dependencies: make required type safe

cf97ea9

interpreter: port dependency static to typed_kwargs

3220ee8

Static must be optional, because although the default is static=false, we special case that static : false is different than static being not set in a number of cases.

dependencies: make static type safe

0832864

interpreter: port dependency version to typed_kwargs

01841f1

dependencies: make version type safe

166873d

modules/python: port dependency embed to typed_kwargs

6b10b48

interpreter: remove permittedKwargs from dependency

2dc6548

interpreter: use typed_kwargs for checking disabler dependency

c2d44f4

interpreter: remove dependency_kwargs

a66aaa2

Since it's basically unusued, but the DEPENDENCY_KWS can be used instead

dependencies/detect: rewrite get_dep_indentifier around DEPENDENCY_KWS

9ddd887

This gives us one source of truth, as well as ensure that an empty value and a default value are treated the same.

dcbaker force-pushed the submit/use-typed-kwargs-for-dependency branch from 1f622f6 to 1a25f7e Compare December 14, 2024 00:19

dnicolodi reviewed Dec 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use typed_kwargs with `dependency()` #13995

Use typed_kwargs with `dependency()` #13995

dcbaker commented Dec 9, 2024

dcbaker commented Dec 9, 2024

dnicolodi Dec 10, 2024

dcbaker Dec 10, 2024

dnicolodi Dec 10, 2024

dcbaker Dec 10, 2024 •

edited

Loading

bruchar1 Dec 11, 2024

dcbaker Dec 11, 2024

eli-schwartz Dec 12, 2024

eli-schwartz Dec 12, 2024

eli-schwartz left a comment

dcbaker commented Dec 12, 2024

dcbaker commented Dec 13, 2024

dnicolodi Dec 13, 2024

dcbaker Dec 14, 2024

dnicolodi Dec 13, 2024

dcbaker Dec 14, 2024

dnicolodi Dec 14, 2024

dcbaker Dec 14, 2024 •

edited

Loading

		if not isinstance(static, bool):
		raise DependencyException('Static keyword must be boolean')

Use typed_kwargs with dependency() #13995

Are you sure you want to change the base?

Use typed_kwargs with dependency() #13995

Conversation

dcbaker commented Dec 9, 2024

dcbaker commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbaker Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eli-schwartz left a comment

Choose a reason for hiding this comment

dcbaker commented Dec 12, 2024

dcbaker commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbaker Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

Use typed_kwargs with `dependency()` #13995

Use typed_kwargs with `dependency()` #13995

dcbaker Dec 10, 2024 •

edited

Loading

dcbaker Dec 14, 2024 •

edited

Loading