Add DIA SDK dependency #13638

vid512 · 2024-09-05T21:48:49Z

Tested on Windows with [msvc, clang_cl], building [x86, x86_64, arm, aarch64] targets (latter two via cross-compiling). AFAIK this is all that DIA SDK supports.

Possible issues:

Version of dependency is not provided. The only way I could find would require us to drag in some library for parsing the version out of msdiaXXX.dll resources.
Should the dependency be named 'dia' or 'diasdk'? I am afraid the name 'dia' may be too generic and may conflict with something else in future. This library in particular is very commonly called 'DIA SDK', including the 'SDK' part, maybe for the very same reason.
Trickiest part is how to allow users to copy the runtime DLL to build directory, if they want to. This is not strictly required, but a very nice practical feature, without which an application must ask its users to manually register some DLL somewhere inside Program Files from admin console. To provide a better option, the dependency needs to return DLL path to meson.build script. I may have abused dependency variables here a bit. Currently this dependency returns a "internal" variable 'dll' (despite "internal" variables being only used with internal dependencies?). I am not aware of any other way to return such value from dependency. It works like this:

dia = dependency('diasdk', required: true)
dia_dll_name = dia.get_variable(internal: 'dll')
fs = import('fs')
fs.copyfile( dia_dll_name )

conf = configuration_data()
conf.set('msdia_dll_name', fs.name(dia_dll_name))
configure_file(input: 'config.h.in', output: 'config.h', configuration: conf)

executable('dia_from_dll', ['dia_from_dll.cpp'], dependencies: [dia])

Not sure if this is tolerable, or if there is some better way.

mesonbuild/dependencies/dev.py

bruchar1 · 2024-09-06T17:31:12Z

test cases/diasdk/1 simple/meson.build

@@ -0,0 +1,8 @@
+project('diatest', 'cpp')


I think you should rather add the tests to test cases/windows instead of creating a new test category

Yeah, I think Windows would be appropriate. The other option would be frameworks

dcbaker

This looks pretty good, I've got a few things we could fix up though

mesonbuild/dependencies/dev.py

dcbaker · 2024-09-06T18:03:48Z

test cases/diasdk/1 simple/meson.build

@@ -0,0 +1,8 @@
+project('diatest', 'cpp')


Yeah, I think Windows would be appropriate. The other option would be frameworks

test cases/diasdk/1 simple/meson.build

docs/markdown/Dependencies.md

vid512 · 2024-09-07T09:54:54Z

All requested changes should now be incorporated. Sorry for chaos with commits & reverts - still learning.

I had to do detection of MSVC 'clang' (not 'clang-cl') compiler in a bit more complicated manner, in order to differentiate MSVC clang vs. MinGW clang.

In my tests, building with this dependency is still failing on x86 clang-cl, due to #13643 . Apparently Meson doesn't have a CI/CD testing for that scenario(?). As soon as that issue gets resolved, this problem should disappear as well.

dcbaker

I have a couple more things, mostly around the version I think we need to work out, but I think we're getting close.

mesonbuild/dependencies/dev.py

docs/markdown/Dependencies.md

docs/markdown/snippets/dia_sdk.md

mesonbuild/dependencies/dev.py

bruchar1

LGTM

eli-schwartz · 2024-09-15T03:01:59Z

mesonbuild/dependencies/dev.py

+            mlog.error('DIA SDK is only supported in C and C++ projects')
+            return


You can raise DependencyException('DIA SDK is ....') here instead, and meson will make it look like this:

Dependency lookup for diasdk with method 'system' failed: DIA SDK is only supported in C and C++ projects

eli-schwartz · 2024-09-15T03:09:23Z

mesonbuild/dependencies/dev.py

+    def get_variable(self, *, cmake: T.Optional[str] = None, pkgconfig: T.Optional[str] = None,
+                     configtool: T.Optional[str] = None, internal: T.Optional[str] = None,
+                     default_value: T.Optional[str] = None,
+                     pkgconfig_define: PkgConfigDefineType = None) -> str:
+        if internal == 'dll':
+            return self.dll
+        if default_value is not None:
+            return default_value
+        raise DependencyException(f'Cannot get variable {internal}')


I... get what this is trying to do, but it still seems weird. :D

@dcbaker this is a system dependency piggybacking onto the mechanism for declare_dependency(variables: [...]) support?

Do we want to extend the get_variable interface to cope with system dependencies? consider them equivalent to configtool or internal? Simply expect them to use get_variable('dll') without a type-specific request?

I would prefer to extend with system type. I still think the no type selection thing is an anti-feature, it lulls people into a false sense of "everything will work right all the time!" and it only happens to work in the case of pkg-config and wraps, but keeps them from even thinking about the less common cases like cmake.

eli-schwartz · 2024-09-15T03:12:24Z

test cases/windows/24 diasdk copy dll/meson.build

+endif
+
+dia = dependency('diasdk', required: true)
+dia_dll_name = dia.get_variable(internal: 'dll')


You don't need "internal" here. The get_variable() method accepts a generic variable name, and a type-specific one. The latter is intended to be useful for cases where e.g. the same information is available via both pkg-config and cmake but the name of the pkg-config variable is different from the name of the cmake variable.

(e.g. because cmake loves capitalized variable names.)

It is therefore necessary to pass a different name into get_variable() depending on which type of dependency was found -- or use dep.get_variable(pkgconfig: 'foo', cmake: 'FOO')

vid512 requested a review from jpakkane as a code owner September 5, 2024 21:48

github-advanced-security bot found potential problems Sep 5, 2024

View reviewed changes

mesonbuild/dependencies/dev.py Fixed Show fixed Hide fixed

mesonbuild/dependencies/dev.py Fixed Show fixed Hide fixed

mesonbuild/dependencies/dev.py Fixed Show fixed Hide fixed

bruchar1 reviewed Sep 6, 2024

View reviewed changes

bruchar1 added the OS:windows Winodows OS specific issues label Sep 6, 2024

dcbaker requested changes Sep 6, 2024

View reviewed changes

vid512 requested a review from dcbaker September 7, 2024 09:55

dcbaker requested changes Sep 9, 2024

View reviewed changes

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

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

vid512 force-pushed the diasdk-dependency branch from 52eeb49 to 10f9fa1 Compare September 10, 2024 08:09

vid512 requested a review from dcbaker September 10, 2024 08:14

bruchar1 reviewed Sep 10, 2024

View reviewed changes

bruchar1 approved these changes Sep 11, 2024

View reviewed changes

vid512 force-pushed the diasdk-dependency branch from 482b7ac to 41f0bdf Compare September 14, 2024 08:25

eli-schwartz reviewed Sep 15, 2024

View reviewed changes

vid512 added 6 commits September 20, 2024 09:17

Add get_variable() system dependency

9af9a50

Document get_variable(system)

da88d09

add diasdk dependency

2961137

add description

7514991

add test cases

da10c75

add doc snippet

eb2e95c

vid512 force-pushed the diasdk-dependency branch from 5c5009a to eb2e95c Compare September 20, 2024 07:23

dcbaker approved these changes Sep 21, 2024

View reviewed changes

dcbaker merged commit dbad301 into mesonbuild:master Sep 21, 2024
36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DIA SDK dependency #13638

Add DIA SDK dependency #13638

vid512 commented Sep 5, 2024

bruchar1 Sep 6, 2024

dcbaker Sep 6, 2024

dcbaker left a comment

dcbaker Sep 6, 2024

vid512 commented Sep 7, 2024 •

edited

Loading

dcbaker left a comment

bruchar1 left a comment

eli-schwartz Sep 15, 2024

eli-schwartz Sep 15, 2024

dcbaker Sep 17, 2024 •

edited

Loading

eli-schwartz Sep 15, 2024

		mlog.error('DIA SDK is only supported in C and C++ projects')
		return

Add DIA SDK dependency #13638

Add DIA SDK dependency #13638

Conversation

vid512 commented Sep 5, 2024

bruchar1 Sep 6, 2024

Choose a reason for hiding this comment

dcbaker Sep 6, 2024

Choose a reason for hiding this comment

dcbaker left a comment

Choose a reason for hiding this comment

dcbaker Sep 6, 2024

Choose a reason for hiding this comment

vid512 commented Sep 7, 2024 • edited Loading

dcbaker left a comment

Choose a reason for hiding this comment

bruchar1 left a comment

Choose a reason for hiding this comment

eli-schwartz Sep 15, 2024

Choose a reason for hiding this comment

eli-schwartz Sep 15, 2024

Choose a reason for hiding this comment

dcbaker Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

eli-schwartz Sep 15, 2024

Choose a reason for hiding this comment

vid512 commented Sep 7, 2024 •

edited

Loading

dcbaker Sep 17, 2024 •

edited

Loading