meson: fix MSVC support #2951
meson: fix MSVC support #2951
Conversation
Regression from commit a5f2c45. It is not possible to unconditionally add the asm sources, since not all compilers understand the .s file extension. Specifically for meson, only compilers inheriting from the GNU mixin will allow a .s file at configure time. zstd doesn't support asm for MSVC for the same basic reason; if/when Windows asm support is added, it would involve preprocessing with nasm, most likely.
|
Discovered while attempting to update the meson WrapDB entry for zstd. We have Windows CI, and it failed. |
ghost
mentioned this pull request
|
2 comments :
|
|
Fundamentally, testing this would mean "attempt to build using MSVC" I guess. My understanding was that your build matrix already tests cmake using the MSVC compiler and VS solution files backend, but does not test meson on windows except using mingw-gcc (and the ninja backend). I don't know what a source code solution would be, to the fact that you have source code files that are only valid inputs to certain compilers. Asm isn't portable. The GNU Compiler Collection includes drivers for many programming languages, including C/C++ but also fortran, ada, objc, Java, golang, and, of relevance here, gas. GNU as. An asm dialect specific to GNU, which clang also implements, but MSVC doesn't. Nor, one presumes, do most alternative C compilers unless they happen to inherit from gcc or clang. You cannot pass an invalid source file to a compiler that doesn't accept it. I don't see any way of working around that anywhere other than a build system, because it is the build system that passes files to the compiler. And, one alternative to writing GNU asm is to write nasm.exe-compatible asm, which works everywhere independent of compiler. Of course, that is because you need to use the nasm compiler instead. This will require even more build system modifications, not "source code" modifications, but at least it will be the same build system modifications to each build system. |
|
Regarding #2957 I do not have any clue why the current CI that runs cmake tests on VS + MSVC passed, but conda forge failed. I had originally assumed that it must work in cmake because I dunno, maybe cmake automatically ignores incompatible file extensions. I'm not a heavy cmake user so I wouldn't know. :D The VS solution files presumably don't need to be updated since they can never include the asm source file to begin with (and don't need to do so conditionally). |
|
Is there anything more that you'd like me to do in this PR? |
Probably not, but since we have an ongoing action with regards to |
There was a problem hiding this comment.
This looks good to me.
The file itself can be parsed as valid C (or anything that supports the C preprocessor) when ZSTD_ENABLE_ASM_X86_64_BMI2 isn't 1, because after the C preprocessor runs, the file will be empty.
Is it meson that is erroring, because it doesn't recognize the extension and doesn't pass it to the compiler, or is it the compiler that is erroring?
|
The former. |
After merging facebook#2951 I realized that we will want to explicitly disable assembly when we aren't including the assembly source file. Otherwise, if some non clang/gcc compiler does support assembly, it would fail to build.
|
@eli-schwartz I put up #2972, as I realized a small problem after I merged your PR. Thanks the the PR! |
Regression from commit a5f2c45. It is not possible to unconditionally add the asm sources, since not all compilers understand the .s file extension.
Specifically for meson, only compilers inheriting from the GNU mixin will allow a .s file at configure time.
zstd doesn't support asm for MSVC for the same basic reason; if/when Windows asm support is added, it would involve preprocessing with nasm, most likely.