[NMakeToolchain] Refactoring to expose similar interface than other toolchains & honor build config #12665

SpaceIm · 2022-12-06T13:44:33Z

Changelog: Fix: Refactoring of NMakeToolchain to expose similar attributes than other toolchains, and honor build config like cflags, cxxflags, sharedlinkflags, exelinkflags, defines & compiler_executables.
Docs: conan-io/docs#2848

Refer to the issue that supports this Pull Request.
If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
I've read the Contributing guide.
I've followed the PEP8 style guides for Python code.
I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

^{Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.}

SpaceIm · 2022-12-08T00:30:10Z

conan/tools/microsoft/nmaketoolchain.py

+        conf_compilers = self._conanfile.conf.get("tools.build:compiler_executables", default={}, check_type=dict)
+        if conf_compilers:
+            compilers_mapping = {
+                "AS": "asm",
+                "CC": "c",
+                "CPP": "cpp",
+                "CXX": "cpp",
+                "RC": "rc",
+            }
+            for env_var, comp in compilers_mapping.items():
+                if comp in conf_compilers:
+                    env.define(env_var, conf_compilers[comp])


Out of scope of this PR, but I believe this toolchain should set all these env vars by default for clang-cl if their corresponding tools.build:compiler_executables key is not set.

It would be nice in order to avoid this kind of fragile logic (and broken for clang mingw):
https://github.com/conan-io/conan-center-index/blob/2ecc63a88002b1294f0ea0650f27c6012933aafa/recipes/libjpeg/all/conanfile.py#L33-L35
https://github.com/conan-io/conan-center-index/blob/2ecc63a88002b1294f0ea0650f27c6012933aafa/recipes/libjpeg/all/conanfile.py#L106-L114

SpaceIm · 2023-01-18T11:25:52Z

@memsharded could you review this PR please? It would be nice to guarantee that we can use NMakeToolchain in conan-center and hope honoring all conf from profile to avoid discrepancies in recipes behavior.

memsharded · 2023-01-19T12:12:34Z

conan/tools/microsoft/nmaketoolchain.py

+        env.append("CFLAGS", self._cflags)
+        env.append("CPPFLAGS", self._cxxflags)
+        env.append("CXXFLAGS", self._cxxflags)


Why defining these variables? If they are used, it will be a duplication of flags (they are already defined in CL). If they are not used, why defining them?

I guess they can be removed yes, as you wish. They are official documented macro of NMake: https://learn.microsoft.com/en-us/cpp/build/reference/special-nmake-macros?view=msvc-170

But defining them may improve robustness, I'm not sure. I'm pretty sure it doesn't hurt at least. It deserves extra experimentations since microsoft documentation is poor. Maybe remove them and left a comment?

By the way, from what I understand from microsoft documentation, CL and LINK are not specific to NMake but to cl & link.

memsharded · 2023-01-19T12:13:33Z

conans/test/functional/toolchains/test_nmake_toolchain.py

+    # Rearrange defines to macro / value dict
+    conf_preprocessors = {}
+    for define in defines:
+        if "=" in define:
+            key, value = define.split("=", 1)
+            if not value:
+                conf_preprocessors[key] = "1"
+            else:
+                conf_preprocessors[key] = value
+        else:
+            conf_preprocessors[define] = "1"


instead of re-arranging, provide the defines already in a dict, and compose it into list when necessary

@pytest.mark.parametrize cannot take dict values AFAIK. Do you have an example?

[{"TEST_DEFINITION1": None}, {"TEST_DEFINITION2": "0"}, .........],

I think I've tried and it didn't work. I can try again.

Actually a change like this wouldn't be robust enough.
If I try to inject /DTEST_DEFINITION1 for {"TEST_DEFINITION1": None} as an input, it won't be understood like this by check_exe_run() which would expect {"TEST_DEFINITION1": "1"}. /DTEST_DEFINITION1 is equivalent to /DTEST_DEFINITION1=1, but I also want to test that /DTEST_DEFINITION1 AND /DTEST_DEFINITION1=1 work in NMakeToolchain. I even want to test that /DTEST_DEFINITION1= works. How to disambiguate when I serialize this dictionary for tools.build:defines, and even after when I pass this dictionary to check_exe_run?
Moreover source code generated by gen_function_cpp() is not robust to a true empty definition like /DTEST_DEFINITION1=.

conans/test/functional/toolchains/test_nmake_toolchain.py

memsharded · 2023-01-19T12:16:48Z

conan/tools/microsoft/nmaketoolchain.py

+            compilers_mapping = {
+                "AS": "asm",
+                "CC": "c",
+                "CPP": "cpp",
+                "CXX": "cpp",
+                "RC": "rc",
+            }
+            for env_var, comp in compilers_mapping.items():
+                if comp in conf_compilers:
+                    env.define(env_var, conf_compilers[comp])


NMake not listening to these CC/CXX, etc env-vars, only to CL

It's not what microsoft documentation says: https://learn.microsoft.com/en-us/cpp/build/reference/special-nmake-macros

I tried to make it work while developing the NMakeToolchain and for some reason it didn't work. If it is expected to work then the way to move this forward is:

remove the injection of flags into CL and use the other options flags as their own env-vars,

modify NMakeDeps to be consistent with this

check that tests correctly work, this test is not validating anywhere that flags ["/GL"], ["/GL"], ["/LTCG"], ["/LTCG"] are actually being honored

I've removed CFLAGS, CXXFLAGS and CPPFLAGS, they have no effect by default indeed. I've kept CC & CXX & CPP, I'm pretty sure these ones are listened. By default CPP is cl and if compiler is clang-cl, CPP must be changed.

memsharded · 2023-01-19T12:19:03Z

conan/tools/microsoft/nmaketoolchain.py

-        if cppstd:
-            cppflags.append(cppstd)
-        from conan.tools.microsoft import msvc_runtime_flag
+    def _get_msvc_runtime_flag(self):


Seems unnecessary, the formatting of flag is doing nothing, the msvc_runtime_flag() can be used directly instead of this method.

It does something, it prepends /. It's just a copy paste of an autotoolstoolchain private method.

memsharded · 2023-01-19T13:14:32Z

conans/test/functional/toolchains/test_nmake_toolchain.py

+    # Rearrange defines to macro / value dict
+    conf_preprocessors = {}
+    for define in defines:
+        if "=" in define:
+            key, value = define.split("=", 1)
+            if not value:
+                conf_preprocessors[key] = "1"
+            else:
+                conf_preprocessors[key] = value
+        else:
+            conf_preprocessors[define] = "1"


[{"TEST_DEFINITION1": None}, {"TEST_DEFINITION2": "0"}, .........],

memsharded · 2023-01-19T13:15:48Z

conans/test/functional/toolchains/test_nmake_toolchain.py

+        ("msvc", "190", "dynamic", "14", "Release", [], [], [], [], []),
+        ("msvc", "190", "dynamic", "14", "Release",
+         ["TEST_DEFINITION1", "TEST_DEFINITION2=0", "TEST_DEFINITION3=", "TEST_DEFINITION4=TestPpdValue4"],
+         ["/GL"], ["/GL"], ["/LTCG"], ["/LTCG"]),


Who checks and validates that these ["/GL"], ["/GL"], ["/LTCG"], ["/LTCG"] flags are being honored?

I don't know how to test that in produced binaries. I'm not sure that it's really tested in any other functional test of conan client actually. The best I could do is a unittest to check whether these flags are passed to CL.

Yes, that is true, it is difficult. I have checked other tests for other build systems, and they do check just reading the generated files or the command line. Still worth it, at least guarantee they are there.

Ok, I'll see what I can do

SpaceIm · 2023-01-21T10:37:54Z

NMakeDeps must also be fixed, it doesn't inject cpp_info.defines and cpp_info.system_libs of dependencies. I'm pretty sure it's the reason of these errors in conan-io/conan-center-index#15386 (since cpp_info.defines are not propagated and GDAL depends on libjpeg static by default in c3i builds, LIBJPEG_STATIC is not injected, therefore GDAL compiles with a wrong mangling of libjpeg interface induced by _delcspec(dllimport), then it fails afterwards at link time in test package).
I've opened another PR: #12944

conan/tools/microsoft/nmaketoolchain.py

honor conf compiler & linker flags in NMakeToolchain

336a8a2

SpaceIm force-pushed the nmaketoolchain-honor-conflags branch 3 times, most recently from baf21f1 to b8af0a4 Compare December 6, 2022 16:38

extend NMakeToolchain test

52e2110

SpaceIm force-pushed the nmaketoolchain-honor-conflags branch from b8af0a4 to 52e2110 Compare December 6, 2022 17:52

SpaceIm changed the title ~~[NMakeToolchainhonor] Honor compiler & linker flags from [conf]~~ [NMakeToolchain] Honor compiler & linker flags from [conf] Dec 6, 2022

SpaceIm added 4 commits December 7, 2022 00:19

refactoring of NMakeToolchain

5413073

inject build type link flags

b758b32

workaround for potentially circular imports?

a120f82

import from visual

cc24e98

SpaceIm changed the title ~~[NMakeToolchain] Honor compiler & linker flags from [conf]~~ [NMakeToolchain] Refactoring to expose similar interface than other toolchains & honor build config Dec 6, 2022

SpaceIm added 11 commits December 7, 2022 02:01

formatting

2ff8e7a

simplify

aa80cd6

typo

ba890ab

define _LINK_ env var instead of LINK

af2b47c

use /nologo instead of /NOLOGO

3178420

add more tests

70ec181

fix preprocessors injection in CL when value is a string

1f800be

add comments

e9cd1bc

handle preprocessor with empty value

fb5ffd1

restore msvc versions in tests

d7959e8

rewording

058835a

SpaceIm mentioned this pull request Dec 7, 2022

[NMakeToolchain] Document new attributes, methods and behavior conan-io/docs#2848

Merged

SpaceIm added 2 commits December 7, 2022 18:24

expose less attributes

3bdd793

typo

5463d79

SpaceIm commented Dec 8, 2022

View reviewed changes

franramirez688 self-requested a review January 18, 2023 12:47

franramirez688 assigned memsharded Jan 18, 2023

franramirez688 added this to the 1.58 milestone Jan 18, 2023

memsharded reviewed Jan 19, 2023

View reviewed changes

SpaceIm commented Jan 21, 2023

View reviewed changes

conan/tools/microsoft/nmaketoolchain.py Outdated Show resolved Hide resolved

SpaceIm mentioned this pull request Jan 21, 2023

gdal < 3.5 : conan v2 support conan-io/conan-center-index#15386

Merged

3 tasks

SpaceIm added 4 commits January 21, 2023 18:53

properly handle empty definitions

6aed528

don't test again stuff already checked by check_exe_run

6054fa9

restore msvc versions in tests

48bb908

remove CFLAGS, CPPFLAGS, CXXFLAGS

b51a66c

memsharded approved these changes Jan 30, 2023

View reviewed changes

memsharded merged commit 2e43345 into conan-io:develop Jan 30, 2023

SpaceIm deleted the nmaketoolchain-honor-conflags branch January 30, 2023 13:39

SpaceIm mentioned this pull request Feb 1, 2023

[package] nasm/2.15.05: AttributeError thrown in Recipe when building under MSVC conan-io/conan-center-index#15637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NMakeToolchain] Refactoring to expose similar interface than other toolchains & honor build config #12665

[NMakeToolchain] Refactoring to expose similar interface than other toolchains & honor build config #12665

SpaceIm commented Dec 6, 2022 •

edited by czoido

Loading

SpaceIm Dec 8, 2022 •

edited

Loading

SpaceIm commented Jan 18, 2023

memsharded Jan 19, 2023

SpaceIm Jan 19, 2023

memsharded Jan 19, 2023

SpaceIm Jan 19, 2023

memsharded Jan 19, 2023

SpaceIm Jan 19, 2023

SpaceIm Jan 21, 2023 •

edited

Loading

memsharded Jan 19, 2023

SpaceIm Jan 19, 2023

memsharded Jan 19, 2023 •

edited

Loading

SpaceIm Jan 21, 2023

memsharded Jan 19, 2023

SpaceIm Jan 19, 2023

memsharded Jan 19, 2023

memsharded Jan 19, 2023

SpaceIm Jan 19, 2023

memsharded Jan 19, 2023

SpaceIm Jan 21, 2023

SpaceIm commented Jan 21, 2023 •

edited

Loading

[NMakeToolchain] Refactoring to expose similar interface than other toolchains & honor build config #12665

[NMakeToolchain] Refactoring to expose similar interface than other toolchains & honor build config #12665

Conversation

SpaceIm commented Dec 6, 2022 • edited by czoido Loading

SpaceIm Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

SpaceIm commented Jan 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

memsharded Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm commented Jan 21, 2023 • edited Loading

SpaceIm commented Dec 6, 2022 •

edited by czoido

Loading

SpaceIm Dec 8, 2022 •

edited

Loading

SpaceIm Jan 21, 2023 •

edited

Loading

memsharded Jan 19, 2023 •

edited

Loading

SpaceIm commented Jan 21, 2023 •

edited

Loading