Skip to content
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

fix another regression in converting build_target kwargs to typed_kwargs #12508

Merged

Conversation

eli-schwartz
Copy link
Member

This time we have a case where people are passing non-objects by using them as str | File, which we never warned about and silently accepted. If it was passed via custom_target outputs we would error out, interestingly enough. At the backend layer, we just pass them directly to the linker... which is valid, if we misdetected what's a valid linker input or people just used funny names. In particular, the mingw toolchain allows passing a *.def file directly, and some people are doing that.

If we do want to allow this, we should do it consistently. For now, just follow the current theme of what's expected, but do so by warning instead of fatally erroring, for cases where users were able to do it in the past.

This time we have a case where people are passing non-objects by using
them as str | File, which we never warned about and silently accepted.
If it was passed via custom_target outputs we *would* error out,
interestingly enough. At the backend layer, we just pass them directly
to the linker... which is valid, if we misdetected what's a valid linker
input or people just used funny names. In particular, the mingw
toolchain allows passing a *.def file directly, and some people are
doing that.

If we do want to allow this, we should do it consistently. For now, just
follow the current theme of what's expected, but do so by warning
instead of fatally erroring, for cases where users were able to do it in
the past.
Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sigh. of course people would do something like pass a .def via objects: instead of the dedicated vs_module_def argument...

@jpakkane jpakkane merged commit cddf2e9 into mesonbuild:master Nov 13, 2023
34 checks passed
@HansKristian-Work
Copy link

HansKristian-Work commented Nov 16, 2023

@dcbaker

Sigh. of course people would do something like pass a .def via objects: instead of the dedicated vs_module_def argument...

Meson ignores vs_module_def when compiling against MinGW, and at least one known workaround is to use objects:.

The docs suggests that using it with MinGW should work though. It mentions Windows, not MSVC in particular, but the name "vs_" does seem MSVC specific.

Specify a Microsoft module definition file for controlling symbol exports, etc., on platforms where that is possible (e.g. Windows).

@dcbaker
Copy link
Member

dcbaker commented Nov 16, 2023

@HansKristian-Work That's a pretty serious bug for us, vs_module_defs should work for mingw. You're right about the name too. Do you know if there's an opened issue for that already, otherwise we need one. We should fix both that it doesn't work on MinGW, and update the documentation to specifically mention MinGW.

@eli-schwartz
Copy link
Member Author

Cross-posting my comments from the linked issue:

I was a bit afraid of that but I didn't have the toolchain to build this locally.

It's a confusing situation since it was silently allowed for source files but not permitted for custom_target outputs... We really should be consistent and allow both. Well actually we should fix vs_module_defs to work correctly.

(I wish I'd known about this mingw failure mode a long time ago.)

@eli-schwartz eli-schwartz deleted the buildtarget-kwargs-regression branch November 17, 2023 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants