Static/dynamic CRT build option #1380
Conversation
CMakeLists.txt
Outdated
| @@ -12,6 +12,7 @@ option(UNIT_TESTING_MOCKS "wrap PAL functions with mock implementation for tests | |||
| option(TRANSPORT_PAHO "Build IoT Samples with Paho MQTT support" OFF) | |||
| option(PRECONDITIONS "Build SDK with preconditions enabled" ON) | |||
| option(LOGGING "Build SDK with logging support" ON) | |||
| option(STATIC_CRT "(MSVC only) build SDK with static CRT" ON) | |||
There was a problem hiding this comment.
| option(STATIC_CRT "(MSVC only) build SDK with static CRT" ON) | |
| option(STATIC_CRT "(MSVC only) Build SDK with static CRT" ON) |
There was a problem hiding this comment.
I wonder if we really want to make this a first-class option in our SDK when it only impacts Windows and isn't specifically related to the SDK itself. Many of the other options have a lot more relevant usage to the end user.
Maybe, we just allow and pass-through any user-provided cmake flags and other arguments in general, and document that if the user wants dynamic CRT, they can add "/MD" themselves to the cmake command.
Alternatively, we can consider defaulting to /MT[d] (i.e. STATIC_CRT = ON) and hold-off on exposing it in the options untli we hear customer feedback or requests for it.
Once we settle on whether we want this, and if you strongly think turning STATIC_CRT off will be an important scenario that warrants an option, consider adding some docs to the README so users know when to turn it off.
There was a problem hiding this comment.
I'd keep it. Being a C dev myself, it is a choice you make for your entire program, and I would not like if a library would limit me in what choice regarding CRT I should make (and it is likely I would pick a DLL CRT). It is not the end of the world, so I would go and patch Azure SDK's CMakeFile, but then the SDK is no longer a black box to me. So I would go one step further.
So, let me to document it.
There was a problem hiding this comment.
Up to you, and I would like @RickWinter to chime in on this too. My concern is that this option is Windows only, which isn't the primary use-case and it makes it seem that the SDK has windows-specific behavior. Outside of the PAL, it seems odd to have such OS-specific options front-and-center.
Any motivated C dev that wants to make such changes, they are already able to do so with simple modifications. This option is trying to make something easier that we don't have evidence users would need. What happens if a customer wants more control and cmake flags that are specific to an OS they are deploying to or their bespoke/specific scenario? Would we add more options for that? In such cases, the customer will likely have to modify the cmakelist anyway, so the SDK build structure can't practically be a blackbox on these edge cases anyway. We should, instead, rely on a general purpose escape hatch to customize cmake (let any custom cmake flag and argument pass through, include /MD).
All that said, I am recommending just deferring until customers actually provide this feedback of the provided cmakelist and we see the value of such an option first hand. Adding an option with that evidence will make the choice clear, and doing it too eagerly will introduce something which might never get used, in practice (and we won't really know it either).
There was a problem hiding this comment.
I I was writing an app, I would want it.
CRT does not only affect PAL. CRT is memmove(), strlen(), isdigit(), sscanf(), modf() used by span, isdigit() used by response, isxdigit() used by json reader, (BTW, I just found out that json_writer probably does not need to include math.h).
There was a problem hiding this comment.
CRT does not only affect PAL.
Yes, of course. I am referring to the number of concepts the user needs to know about that convey abstraction on top of the hardware/OS/compiler? We just have one, which is the PAL.
I don't think we should introduce compiler and OS specific flags to our options, since these are not relevant concepts for the use of the Azure SDK. The SDK should remain agnostic to such differences.
There was a problem hiding this comment.
Fixing the triplet to use the correct package based on our build configurations is correct. Adding support for the customer to build static and dynamic should be done only if the customer demand is present.
Have you talked with Yuxin and Dane to get input on their champion scenarios. Are customers going to build static and dynamic, or will they focus on one. Is one or the other a better experience and thus its in our best interest to steer the solution to the ideal place by only supporting 1 model.
Two notes:
- Its unclear to me why we would only add static/dynamic linking support on Windows w/ msvc.
- Adding this support does increase the matrix of configurations in the pipeline. We will need to build/test/support these moving forward.
/MT, /MTd, /MD, /MDd.
There was a problem hiding this comment.
For some additional context, the -md flavors of the triplet aren't officially supported by vcpkg and using that gives a warning from vcpkg, on install, which is one of the reasons I suggested we continue to use x64-windows-static:
-- Using community triplet x64-windows-static-md. This triplet configuration is not guaranteed to succeed.
If linux/mac default to dynamic linking (and if that's what our customers want), we should use these triplets on Windows to match and have consistent behavior across the board (and update the docs):
https://github.com/microsoft/vcpkg/blob/master/triplets/x64-windows.cmake
https://github.com/microsoft/vcpkg/blob/master/triplets/x86-windows.cmake
There was a problem hiding this comment.
I would definitely be interested to hear what our IoT partners think about static vs dynamic linking. @yuxin-azrtos, @danewalton, @hihigupt
https://stackoverflow.com/questions/30675681/embedded-systems-static-or-dynamic-linking
So it implies that dynamic linking is possible only if:
- You have a some kind of OS
- You have some kind of persistent storage / file system.
On a bare-metal micros it is usually not the case.
The embedded systems RTOS VxWorks supports dynamic linking in the sense that it can load and link partially linked object code from a network or file system at runtime. Similarly Larger embedded RTOSs such as QNX support dynamic linking, as does embedded Linux.
So yes large embedded systems may support dynamic linking.
https://en.wikibooks.org/wiki/Embedded_Systems/C_Programming
Few embedded systems have capability for dynamic linking, so if standard library functions are to be available at all, they often need to be directly linked into the executable.
There is also another factor involving binary size vs compile time.
Also tagging @JeffreyRichter as an FYI.
antkmsft
requested review from
danieljurek,
joshfree,
RickWinter and
a team
as code owners
README.md
Outdated
| @@ -189,6 +189,11 @@ The following CMake options are available for adding/removing project features. | |||
| <td>This option can be set to any of the next values:<br>- No_value: default value is used and no_platform library is used.<br>- "POSIX": Provides implementation for Linux and Mac systems.<br>- "WIN32": Provides platform implementation for Windows based system<br>- "USER": Tells cmake to use an specific implementation provided by user. When setting this option, user must provide an implementation library and set option `AZ_USER_PLATFORM_IMPL_NAME` with the name of the library (i.e. <code>-DAZ_PLATFORM_IMPL=USER -DAZ_USER_PLATFORM_IMPL_NAME=user_platform_lib</code>). cmake will look for this library to link az_core</td> | |||
| <td>No_value</td> | |||
| </tr> | |||
| <tr> | |||
| <td>STATIC_CRT</td> | |||
| <td>This option has effect only when MSVC is used. It specifies, whether the SDK should be built with the static (ON) or the dynamic (OFF) C runtime dependency.</td> | |||
There was a problem hiding this comment.
| <td>This option has effect only when MSVC is used. It specifies, whether the SDK should be built with the static (ON) or the dynamic (OFF) C runtime dependency.</td> | |
| <td>This option has effect only when the MSVC compiler is used, specifically on Windows. It specifies whether the SDK should be built with the static (ON) or the dynamic (OFF) C runtime dependency.</td> |
There was a problem hiding this comment.
Affect is verb, effect is noun. "Affects something", but "has effect", so it would need to be something like "Affects compilation when MSVC compiler is used".
I agree to "the MSVC compiler", but we don't need the "specifically on Windows" part. MSVC is Windows-only.
There was a problem hiding this comment.
Our support matrix is generally in terms of supported OSes (which dictate the default compiler and version). I wouldn't expect users to know what MSVC is and that MSVC is a Windows only compiler.
What we want to convey is: If I am not running on Windows, this option is not useful.
Affect is verb, effect is noun. "Affects something", but "has effect"
You are probably right. Updated. I used the short-hand if "result" can be substituted in for effect, in this case, and it couldn't.
Also, don't forget to remove the comma after It specifies,
There was a problem hiding this comment.
I wouldn't expect users to know what MSVC is and that MSVC is a Windows only compiler.
So, you read "This option has effect only when the MSVC compiler is used", and you don't know what it is, then you are not using it, so you can skip reading the rest, the option does not apply to you.
What we want to convey is: If I am not running on Windows, this option is not useful.
If you are using Intel compiler, for example, the option does not apply either.
There was a problem hiding this comment.
Regarding the comma, I don't think it is needed, just look how many occurrences are not using a comma there: https://www.bing.com/search?q=%22it+specifies%2C+whether%22&form=QBLH&sp=-1&pq=%22it+specifies%2C+whether%22&sc=0-23&qs=n&sk=&cvid=BD9996618C914542A4640B89E508E584
There was a problem hiding this comment.
I don't think it is needed, just look how many occurrences are not using a comma there:
I think we both are saying the same thing (i.e. remove the comma) :)
If you are using Intel compiler, for example, the option does not apply either.
Is that a specific compiler you are referring to? What is the name of that Intel compiler (and what OS/architecture)?
There was a problem hiding this comment.
ICC, supports C99, works on Windows and MacOS.
Co-authored-by: Ahson Khan <[email protected]>
| @@ -45,6 +46,17 @@ if(DEFINED ENV{VCPKG_DEFAULT_TRIPLET} AND NOT DEFINED VCPKG_TARGET_TRIPLET) | |||
| endif() | |||
|
|
|||
| project(az LANGUAGES C) | |||
|
|
|||
| if (MSVC) | |||
Follow up on #1338.
The fully and officially supported VCPKG triplet is with static CRT. Our docs mention tat triplet.
Dynamic CRT is also pretty popular. So, we make an option to select, and the default is the one that works with vcpkg, and requires minimum explanations, but the very next step some users might want to do - we also got them covered, so that they don't have to dig through cmakefile if they simply want dynamic CRT.