Add tools.build:asmflags configuration setting for cmake toolchain #17237
Conversation
jwidauer
force-pushed
the
add-asm-flags-conf-flag
branch
from
460b2d1 to
52567e1
Compare
| @@ -343,6 +344,7 @@ class ParallelBlock(Block): | |||
|
|
|||
| string(APPEND CONAN_CXX_FLAGS " /MP{{ parallel }}") | |||
| string(APPEND CONAN_C_FLAGS " /MP{{ parallel }}") | |||
| string(APPEND CONAN_ASM_FLAGS " /MP{{ parallel }}") | |||
There was a problem hiding this comment.
TIL, do you have any link that shows that MP also applies to ASM?
There was a problem hiding this comment.
I don't think this option is really language specific, since it just specifies how many processes should be used to compile the sources. I had a look here and it doesn't specifically mention assembly anywhere. But it's under the "C++, C and Assembler" section, leading me to believe it should also apply.
This addition just makes sure the flag is also supplied to assembly targets.
There was a problem hiding this comment.
I think we would need a bit more of evidence, for example https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS.html#variable:CMAKE_%3CLANG%3E_FLAGS it is not listing it, and in other places, it seems that it requires de definition of a dialect. Otherwise, it might be better to leave it out at the moment.
There was a problem hiding this comment.
Ah, do you mean just CMAKE_ASM_FLAGS in general? That comes from the CMAKE_<LANG>_FLAGS_INIT flag, with <LANG> == ASM, which is a supported option, as you can see from the enable_language() documentation (also from the project(...) one, but easier to find in the former).
Or am I misunderstanding you and you just mean the /MP flag in particular?
There was a problem hiding this comment.
Actually both. If CMAKE_ASM_FLAGS is defined with value, will it apply to all potential dialects ASM_NASM, ASM_MARMASM, ASM_MASM, and ASM-ATT.?
Does the assembler really works with /MP flag? Is it possible that it might produce some error? Indeed the only reference I can find is https://learn.microsoft.com/en-us/cpp/build/reference/mp-build-with-multiple-processes?view=msvc-170, which seems it applies to the msvc assembler.
My concern is adding something that is not covered by tests, not used or check in our CI, etc. It is not the first time we are impacted by an apparently evident addition, just to find there are cases that it breaks. As this PR is about adding the possibility to inject flags for asm from conf, changing this might be a slightly unrelated issue.
If you tell me that you tested and confirmed it locally, for example by checking the verbose command line arguments, that the /MP is being used correctly and without problems, then we might move forward, but it would need at least some confirmation of manual testing.
There was a problem hiding this comment.
Hi @jwidauer
Any feedback about this last comment? Thanks!
There was a problem hiding this comment.
Hi @memsharded
Sorry for the long silence. I got caught up with other stuff at work!
I have now tried the /MP flag using CMake, MSVC and an assembly file and it all works correctly and doesn't produce any errors!
Does that satisfy your concerns or would you want me to do anything else?
Changelog: Feature
The CMake toolchain supports the
tools.build:cflagsandtools.build:cxxflagsconfiguration settings, which then populate theCMAKE_C_FLAGS_INITand theCMAKE_CXX_FLAGS_INITCMake variables respectively. There doesn't seem to exist an equivalent for theCMAKE_ASM_FLAGS_INITCMake variable in conan, right now.This PR adds the new
tools.build:asmflagsconfiguration setting, to address this.In this PR I only included the necessary changes for the CMake toolchain, but I could also add support for this setting in some of the other toolchain generators, if there's interest.
Docs: conan-io/docs#3881
developbranch, documenting this one.