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.

Sign up for GitHub

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

Add tools.build:asmflags configuration setting for cmake toolchain #17237

Open
wants to merge 2 commits into
base: develop2
Choose a base branch
from
Open

Add tools.build:asmflags configuration setting for cmake toolchain #17237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
52567e1
feat: add tools.build:asmflags configuration setting for cmake toolchain
jwidauer Oct 28, 2024
fa9bd95
fixup! feat: add tools.build:asmflags configuration setting for cmake…
jwidauer Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion conan/tools/cmake/toolchain/blocks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,11 +233,12 @@ def context(self):

class ArchitectureBlock(Block):
template = textwrap.dedent("""\
# Define C++ flags, C flags and linker flags from 'settings.arch'
# Define C++ flags, C flags, ASM flags and linker flags from 'settings.arch'
message(STATUS "Conan toolchain: Defining architecture flag: {{ arch_flag }}")
string(APPEND CONAN_CXX_FLAGS " {{ arch_flag }}")
string(APPEND CONAN_C_FLAGS " {{ arch_flag }}")
string(APPEND CONAN_ASM_FLAGS " {{ arch_flag }}")
string(APPEND CONAN_SHARED_LINKER_FLAGS " {{ arch_flag }}")
string(APPEND CONAN_EXE_LINKER_FLAGS " {{ arch_flag }}")
""")
Expand Down Expand Up @@ -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 }}")
Copy link
Member

Choose a reason for hiding this comment

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

TIL, do you have any link that shows that MP also applies to ASM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @jwidauer

Any feedback about this last comment? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

""")

def context(self):
Expand Down Expand Up @@ -729,6 +731,9 @@ class ExtraFlagsBlock(Block):
{% if cflags %}
string(APPEND CONAN_C_FLAGS{{suffix}} "{% for cflag in cflags %} {{ cflag }}{% endfor %}")
{% endif %}
{% if asmflags %}
string(APPEND CONAN_ASM_FLAGS{{suffix}} "{% for asmflag in asmflags %} {{ asmflag }}{% endfor %}")
{% endif %}
{% if sharedlinkflags %}
string(APPEND CONAN_SHARED_LINKER_FLAGS{{suffix}} "{% for sharedlinkflag in sharedlinkflags %} {{ sharedlinkflag }}{% endfor %}")
{% endif %}
Expand Down Expand Up @@ -783,6 +788,7 @@ def context(self):
# Now, it's time to get all the flags defined by the user
cxxflags = self._toolchain.extra_cxxflags + self._conanfile.conf.get("tools.build:cxxflags", default=[], check_type=list)
cflags = self._toolchain.extra_cflags + self._conanfile.conf.get("tools.build:cflags", default=[], check_type=list)
asmflags = self._toolchain.extra_asmflags + self._conanfile.conf.get("tools.build:asmflags", default=[], check_type=list)
sharedlinkflags = self._toolchain.extra_sharedlinkflags + self._conanfile.conf.get("tools.build:sharedlinkflags", default=[], check_type=list)
exelinkflags = self._toolchain.extra_exelinkflags + self._conanfile.conf.get("tools.build:exelinkflags", default=[], check_type=list)
defines = self._conanfile.conf.get("tools.build:defines", default=[], check_type=list)
Expand All @@ -805,6 +811,7 @@ def context(self):
"suffix": suffix,
"cxxflags": cxxflags,
"cflags": cflags,
"asmflags": asmflags,
"sharedlinkflags": sharedlinkflags,
"exelinkflags": exelinkflags,
"defines": [define.replace('"', '\\"') for define in defines]
Expand All @@ -823,6 +830,9 @@ class CMakeFlagsInitBlock(Block):
if(DEFINED CONAN_C_FLAGS_${config})
string(APPEND CMAKE_C_FLAGS_${config}_INIT " ${CONAN_C_FLAGS_${config}}")
endif()
if(DEFINED CONAN_ASM_FLAGS_${config})
string(APPEND CMAKE_ASM_FLAGS_${config}_INIT " ${CONAN_ASM_FLAGS_${config}}")
endif()
if(DEFINED CONAN_SHARED_LINKER_FLAGS_${config})
string(APPEND CMAKE_SHARED_LINKER_FLAGS_${config}_INIT " ${CONAN_SHARED_LINKER_FLAGS_${config}}")
endif()
Expand All @@ -837,6 +847,9 @@ class CMakeFlagsInitBlock(Block):
if(DEFINED CONAN_C_FLAGS)
string(APPEND CMAKE_C_FLAGS_INIT " ${CONAN_C_FLAGS}")
endif()
if(DEFINED CONAN_ASM_FLAGS)
string(APPEND CMAKE_ASM_FLAGS_INIT " ${CONAN_ASM_FLAGS}")
endif()
if(DEFINED CONAN_SHARED_LINKER_FLAGS)
string(APPEND CMAKE_SHARED_LINKER_FLAGS_INIT " ${CONAN_SHARED_LINKER_FLAGS}")
endif()
Expand Down
1 change: 1 addition & 0 deletions conan/tools/cmake/toolchain/toolchain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ def __init__(self, conanfile, generator=None):

self.extra_cxxflags = []
self.extra_cflags = []
self.extra_asmflags = []
self.extra_sharedlinkflags = []
self.extra_exelinkflags = []

Expand Down
1 change: 1 addition & 0 deletions conans/model/conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@
"tools.build:compiler_executables": "Defines a Python dict-like with the compilers path to be used. Allowed keys {'c', 'cpp', 'cuda', 'objc', 'objcxx', 'rc', 'fortran', 'asm', 'hip', 'ispc'}",
"tools.build:cxxflags": "List of extra CXX flags used by different toolchains like CMakeToolchain, AutotoolsToolchain and MesonToolchain",
"tools.build:cflags": "List of extra C flags used by different toolchains like CMakeToolchain, AutotoolsToolchain and MesonToolchain",
"tools.build:asmflags": "(Experimental) List of extra assembly flags used by the CMakeToolchain generator",
"tools.build:defines": "List of extra definition flags used by different toolchains like CMakeToolchain, AutotoolsToolchain and MesonToolchain",
"tools.build:sharedlinkflags": "List of extra flags used by different toolchains like CMakeToolchain, AutotoolsToolchain and MesonToolchain",
"tools.build:exelinkflags": "List of extra flags used by different toolchains like CMakeToolchain, AutotoolsToolchain and MesonToolchain",
Expand Down
2 changes: 1 addition & 1 deletion test/integration/configuration/conf/test_conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ def test_composition_conan_conf_different_data_types_by_cli_arg(client):
"""
Testing if you want to introduce a list/dict via cli
>> conan install . -c "tools.build.flags:ccflags+=['-Werror']"
>> conan install . -c "tools.build:cflags+=['-Werror']"
>> conan install . -c "tools.microsoft.msbuildtoolchain:compile_options={'ExceptionHandling': 'Async'}"
"""
Expand Down
5 changes: 5 additions & 0 deletions test/integration/toolchains/cmake/test_cmaketoolchain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,7 @@ def test_extra_flags_via_conf():
[conf]
tools.build:cxxflags=["--flag1", "--flag2"]
tools.build:cflags+=["--flag3", "--flag4"]
tools.build:asmflags+=["--flag3", "--flag4"]
tools.build:sharedlinkflags=+["--flag5", "--flag6"]
tools.build:exelinkflags=["--flag7", "--flag8"]
tools.build:defines=["D1", "D2"]
Expand All @@ -513,6 +514,7 @@ def test_extra_flags_via_conf():
toolchain = client.load("conan_toolchain.cmake")
assert 'string(APPEND CONAN_CXX_FLAGS " --flag1 --flag2")' in toolchain
assert 'string(APPEND CONAN_C_FLAGS " --flag3 --flag4")' in toolchain
assert 'string(APPEND CONAN_ASM_FLAGS " --flag3 --flag4")' in toolchain
assert 'string(APPEND CONAN_SHARED_LINKER_FLAGS " --flag5 --flag6")' in toolchain
assert 'string(APPEND CONAN_EXE_LINKER_FLAGS " --flag7 --flag8")' in toolchain
assert 'add_compile_definitions( "D1" "D2")' in toolchain
Expand Down Expand Up @@ -1327,6 +1329,7 @@ def generate(self):
tc = CMakeToolchain(self)
tc.extra_cxxflags = ["extra_cxxflags"]
tc.extra_cflags = ["extra_cflags"]
tc.extra_asmflags = ["extra_asmflags"]
tc.extra_sharedlinkflags = ["extra_sharedlinkflags"]
tc.extra_exelinkflags = ["extra_exelinkflags"]
tc.generate()
Expand All @@ -1336,6 +1339,7 @@ def generate(self):
[conf]
tools.build:cxxflags+=['cxxflags']
tools.build:cflags+=['cflags']
tools.build:asmflags+=['asmflags']
tools.build:sharedlinkflags+=['sharedlinkflags']
tools.build:exelinkflags+=['exelinkflags']
""")
Expand All @@ -1345,6 +1349,7 @@ def generate(self):

assert 'string(APPEND CONAN_CXX_FLAGS " extra_cxxflags cxxflags")' in toolchain
assert 'string(APPEND CONAN_C_FLAGS " extra_cflags cflags")' in toolchain
assert 'string(APPEND CONAN_ASM_FLAGS " extra_asmflags asmflags")' in toolchain
assert 'string(APPEND CONAN_SHARED_LINKER_FLAGS " extra_sharedlinkflags sharedlinkflags")' in toolchain
assert 'string(APPEND CONAN_EXE_LINKER_FLAGS " extra_exelinkflags exelinkflags")' in toolchain

Expand Down