Rust module enhancements for mesa #11902

dcbaker · 2023-06-22T17:42:24Z

These are issues that Mesa has run into with Rust in Meson. This is still a draft because docs and tests are missing, but I wanted to put this is up because it's really important for mesa to land these for 1.2.0, so I wanted to get it on the milestone and on the radar

mesonbuild/modules/rust.py

lazka · 2023-06-23T07:54:44Z

(fyi, the msys2 CI errors should be fixed by a simple rerun of those jobs)

docs/markdown/snippets/rust_extra_clang_bindgen_arguments.md

eli-schwartz · 2023-06-26T17:55:30Z

mesonbuild/envconfig.py

+        value = mesonlib.listify(self.properties.get('bindgen_clang_arguments', []))
+        if not all(isinstance(v, str) for v in value):
+            raise EnvironmentException('bindgen_clang_arguments must be a string or an array of strings')
+        return T.cast('T.List[str]', value)


Shouldn't this be stringlistify, which incidentally returns T.List[str] for you already?

stringlistify has such a generic error message that it's basically useless, so we have to catch that exception, throw it away, and raise a new exception that is both the right type and has a useful error message. I can make that change, but it doesn't seem that useful

try: return mesonlib.stringlistify(self.properties.get('bindgen_clang_arguments', [])) except MesonException: raise EnvironmentException('Useful error message') from None

Hmm, bit disappointing... maybe we can just make stringlistify/typeslistify accept a prefix= argument that will be prepended to "(List) item must be" in error messages?

AFAICT basically every use of typeslistify/stringlistify should go away when we finish the typed_kwargs conversion, because they're basically all things like: x = stringlistyf(extract_as_list(kwargs, 'foo')). typeslistify also doesn't flatten, so we're still stuck with mesonlib.stringlistify(mesonlib.listify(self.properties.get('bindgen_clang_arguments', []))). The error message is still not great, even with prefix.

Do we even normally expect flattening in a machine file, where it's not standard to have things that may be strings, may be arrays, and may be combined using various operators some of which append and some of which extend?

Plus, we're still having to catch it to convert the type from a generic MesonException to EnvironmentException.

(But actually I do think the error message from typeslistify is a strict improvement modulo the prefix :p)

Do we even normally expect flattening in a machine file

Yes. Or we don't do any validation and die in a fire, or we just assert a bunch of stuff with no messages :D

But for things that may be arrays listify() is in fact called

I opened #11912 for the prefix thing. Since this PR is super important for mesa and the merge window is about to close, I'd really like to get this in today. If you want me to switch to using stringlistify, I'd rather do that as part of #11912 after this lands.

mesonbuild/backend/ninjabackend.py

eli-schwartz · 2023-06-27T00:47:14Z

docs/markdown/Rust-module.md

@@ -18,7 +18,7 @@ like Meson, rather than Meson work more like rust.

 ## Functions

-### test(name: string, target: library | executable, dependencies: []Dependency)
+### test(name: string, target: library | executable, dependencies: []Dependency, link_with: []targets)


I continue to find this an unpleasant and ugly way to document anything.

e.g. https://mesonbuild.com/Rust-module.html#bindgen-input-string-buildtarget-string-buildtarget-output-string-include_directories-include_directories-string-c_args-string-args-string-dependencies-dependency is totally unreadable and also impossible to stably link to.

It's also an out-and-out lie, because rust_mod.test(name: 'foobar', target: mytarget) isn't a valid invocation approach, but "dependencies" is a kwarg rather than a typed_pos_arg, and this documentation asserts a positive statement that they are all of them kwargs.

No one is using this information, because they literally cannot (figure out how to read it).

This is not a blocking review comment, just an existing issue that I thought I'd mention while I'm here.

I know you don't like it, but at this point the only realistic way to make all of the module docs consistent is for someone to convert them to the yaml format and generate them them that way, because any other attempt to standardize them just seems like a waste of energy :/

eli-schwartz

I am not an expert on the rust code but it seems okay to me. A couple minor documentation typos that may be worth fixing...

docs/markdown/snippets/rust_extra_clang_bindgen_arguments.md

mesonbuild/modules/rust.py

docs/markdown/snippets/rust_bindegen_extra_args.md

This was requested by Mesa, where a bunch of `declare_dependency` objects are being created as a workaround for the lack of this keyword

…indgen It's currently impossible to inject extra clang arguments when using bindgen, which is problematic when cross compiling since you may need critical arguments like `--target=...`. Because such arguments must be passed after the `--` it's impossible to inject them currently without going to something like a wrapper script. Fixes: mesonbuild#11805

…ompiler This may be necessary to, for example, stop rustc complaining about unused functions

Otherwise we might not get things like libstdc++, which we need.

Rust by default links with the default MSVCRT, (dynamic, release). MSVCRT's cannot be mixed, so if Meson compiles a C or C++ library and links it with the debug MSVCRT, then tries to link that with the Rust library there will be failures. There is no built-in way to fix this for rustc, so as a workaround we inject the correct arguments early in the linker line (before any libs at least) to change the runtime. This seems to work and is recommended as workaround in the upstream rust bug report: rust-lang/rust#39016. Given that this bug report has been opened since 2017, it seems unlikely to be fixed anytime soon, and affects all (currently) released versions of Rust.

xclaesse · 2023-07-04T12:53:26Z

mesonbuild/build.py

@@ -1616,6 +1614,15 @@ def get_clink_dynamic_linker_and_stdlibs(self) -> T.Tuple['Compiler', T.List[str

        raise AssertionError(f'Could not get a dynamic linker for build target {self.name!r}')

+    def get_used_stdlib_args(self, link_language: str) -> T.List[str]:
+        all_compilers = self.environment.coredata.compilers[self.for_machine]
+        all_langs = set(all_compilers).union(self.get_langs_used_by_deps())


This seems wrong to me, please correct me if I misunderstood. The union() here is useless because all_compilers is already all compilers, there is nothing we can add to that set. Secondly you add stdlib link flags of all available languages to all targets regardless whether they are using that language or not. In a project that mix C++ and Fortran this will add -lc++ to a pure Fortrant target AFAIK.

BTW, all_compilers is even all compilers from all subprojects. There is interpreter.compilers which is only compilers from the current subproject.

We already called get_langs_used_by_deps(), instead of calling it again dep_langs should be passed as argument.

I believe this code was correct, the problem is not here but in get_langs_used_by_deps() which should recurse on dependencies.

xclaesse · 2023-07-04T13:18:56Z

mesonbuild/backend/ninjabackend.py

+            elif crt == 'mtd':
+                crt_link_args = ['-l', 'static=libcmtd']
+            elif crt == 'mt':
+                crt_link_args = ['-l', 'static=libcmt']


Shouldn't this be moved to RustCompiler.get_crt_link_args()? NasmCompiler has a very similar case.

dcbaker added the module:rust Specific to the Rust module label Jun 22, 2023

dcbaker added this to the 1.2.0 milestone Jun 22, 2023

dcbaker force-pushed the submit/rust-module-enhancements branch from 7f17fc3 to 7fb21b0 Compare June 22, 2023 17:47

xclaesse reviewed Jun 22, 2023

View reviewed changes

mesonbuild/modules/rust.py Outdated Show resolved Hide resolved

dcbaker force-pushed the submit/rust-module-enhancements branch from 7fb21b0 to 42b303c Compare June 22, 2023 18:58

dcbaker force-pushed the submit/rust-module-enhancements branch 5 times, most recently from 3a7dde4 to 9ad8156 Compare June 23, 2023 22:53

alyssais reviewed Jun 23, 2023

View reviewed changes

docs/markdown/snippets/rust_extra_clang_bindgen_arguments.md Outdated Show resolved Hide resolved

dcbaker force-pushed the submit/rust-module-enhancements branch 2 times, most recently from 90640f4 to 7aab6f0 Compare June 26, 2023 17:52

eli-schwartz reviewed Jun 26, 2023

View reviewed changes

dcbaker force-pushed the submit/rust-module-enhancements branch from 7aab6f0 to e2b2e25 Compare June 26, 2023 17:55

eli-schwartz requested changes Jun 26, 2023

View reviewed changes

mesonbuild/backend/ninjabackend.py Outdated Show resolved Hide resolved

dcbaker force-pushed the submit/rust-module-enhancements branch from e2b2e25 to d4ed1a1 Compare June 26, 2023 19:16

dcbaker marked this pull request as ready for review June 26, 2023 19:17

dcbaker requested a review from jpakkane as a code owner June 26, 2023 19:17

eli-schwartz reviewed Jun 27, 2023

View reviewed changes

eli-schwartz approved these changes Jun 27, 2023

View reviewed changes

docs/markdown/snippets/rust_extra_clang_bindgen_arguments.md Outdated Show resolved Hide resolved

mesonbuild/modules/rust.py Outdated Show resolved Hide resolved

docs/markdown/snippets/rust_bindegen_extra_args.md Outdated Show resolved Hide resolved

dcbaker force-pushed the submit/rust-module-enhancements branch 2 times, most recently from ae8c2f2 to dfdf03f Compare June 27, 2023 18:49

dcbaker added 5 commits June 27, 2023 11:53

modules/rust: Add a link_with kwarg to the test method

43f2406

This was requested by Mesa, where a bunch of `declare_dependency` objects are being created as a workaround for the lack of this keyword

modules/rust: Add a keyword argument to pass extra args to the rust c…

5d16bd5

…ompiler This may be necessary to, for example, stop rustc complaining about unused functions

rust: get stdlib arguments for non-rust languages when linking

772cb92

Otherwise we might not get things like libstdc++, which we need.

dcbaker force-pushed the submit/rust-module-enhancements branch from dfdf03f to 6bfb47a Compare June 27, 2023 18:53

jpakkane merged commit a4fb8dc into mesonbuild:master Jun 27, 2023
35 checks passed

dcbaker deleted the submit/rust-module-enhancements branch June 27, 2023 21:15

xclaesse reviewed Jul 4, 2023

View reviewed changes

This was referenced Oct 13, 2023

rust: Fix link arguments #12367

Merged

rust: Always inject vscrt args #12381

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust module enhancements for mesa #11902

Rust module enhancements for mesa #11902

dcbaker commented Jun 22, 2023

lazka commented Jun 23, 2023 •

edited

eli-schwartz Jun 26, 2023

dcbaker Jun 26, 2023

eli-schwartz Jun 26, 2023

dcbaker Jun 26, 2023 •

edited

eli-schwartz Jun 26, 2023

dcbaker Jun 26, 2023

eli-schwartz Jun 26, 2023

dcbaker Jun 26, 2023

dcbaker Jun 26, 2023

eli-schwartz Jun 27, 2023

dcbaker Jun 27, 2023

eli-schwartz left a comment

xclaesse Jul 4, 2023

xclaesse Jul 4, 2023

xclaesse Jul 4, 2023

xclaesse Jul 4, 2023

Rust module enhancements for mesa #11902

Rust module enhancements for mesa #11902

Conversation

dcbaker commented Jun 22, 2023

lazka commented Jun 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbaker Jun 26, 2023 • edited

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

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

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

lazka commented Jun 23, 2023 •

edited

dcbaker Jun 26, 2023 •

edited