-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
cmake/common: Add language map entry for nasm #13864
base: master
Are you sure you want to change the base?
Conversation
@mensinda, you have been assigned as a reviewer, so I hope you don't mind a question 😅 So first, the symptom this change wants to address is the warning "CMake Toolchain: Failed to determine CMake compilers state" being emitted, in certain circumstances where a subproject uses nasm sources. Now I'd like to add a test that triggers the circumstances of the warning. So... do you happen to have any advice on how to approach this? |
@res2k I don't know if mensinda will answer, they haven't been around for a while. What you likely want to do is use a test.json, which has a field for looking for specific output from the |
@dcbaker, thanks for chipping in. I noticed you could check for the presence of some specific line in the output - Ie, fail case - the specific warning is emitted; success case - the specific warning is not emitted. So I wonder if there's a way to do that with the current system ... |
There is a way to search for a regex, I bet there's a way in python re to invert the condition? |
Actually, I noticed that there's apparently a way to check some output line occurs zero times, which seems to be what I want :) |
1324091
to
2833865
Compare
@dcbaker, I managed to cobble together a test case that does what I want: produces "Failed to determine CMake compilers state" (and fails due to that) without the language map changes, but works warning-less with the latter change. |
fad4514
to
9aa0b11
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for adding this and the test
d78c994
to
92034c1
Compare
The error on the Ubuntu Rolling job looks relevant |
It's odd though. Pretty much looks like just |
Ohhhh, that's awesome. Let me give that job a kick and see what happens. |
Idea: that failing job is cross compiling to ARM, is there a chance that the ASM in question doesn't work an armv7? |
Maybe. But TBH I would expect nasm to tell me that instead of crashing. But I guess I'll disable the test for ARM anyway. Which is fine, I guess, the principle ("we don't write a bogus CMake language") is just as well demonstrated on other platforms. |
...then again, nasm is only "an asssembler for the x86 CPU architecture", so maybe the weird thing is that this cross-to-ARM sometimes worked? |
30d8999
to
e71b010
Compare
nasm targets x86, so enabling it only there makes sense.
To work around weird segfault on Ubuntu Rolling (--cross ubuntu-armhf.json --cross linux-mingw-w64-64bit.json, 0, gcc, g++)
e71b010
to
d8a1d5e
Compare
When determining the CMake compilers state, a
CMakeLists.txt
with these languages would be written:project(CompInfo C NASM CXX)
Unfortunately,
NASM
isn't a CMake language. ButASM_NASM
is - so map to that.