-
-
Notifications
You must be signed in to change notification settings - Fork 570
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
Add ODIN_CRT_MD flag for core:c/libc on windows #3307
base: master
Are you sure you want to change the base?
Conversation
core/c/libc/complex.odin
Outdated
@@ -3,7 +3,11 @@ package libc | |||
// 7.3 Complex arithmetic | |||
|
|||
when ODIN_OS == .Windows { | |||
foreign import libc "system:libucrt.lib" | |||
when ODIN_CRT_MD { |
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.
Why is it called ODIN_CRT_MD
?
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.
I'm open for a better name. The current name is just the ODIN
prefix followed by the subject CRT
and MD
for the C++ equivalent /MD
flag.
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.
How about ODIN_DYNAMIC_CRT
which is what /MD
does as opposed to /MT
that statically link CRT?
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.
DYNAMIC_CRT
is certainly better than CRT_MD
.
core/c/libc/complex.odin
Outdated
@@ -3,7 +3,11 @@ package libc | |||
// 7.3 Complex arithmetic | |||
|
|||
when ODIN_OS == .Windows { | |||
foreign import libc "system:libucrt.lib" | |||
when ODIN_DYNAMIC_CRT { |
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.
The problem here is that it kind of implies that other targets have a static one by default, which is not necessarily the case.
I am not saying what you've done is a bad idea, but rather it's got issues with affect other platforms.
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.
I agree this is a windows only option and having it available on other platforms feels wrong. Is it possible to define a config flag for only one platform? (maybe by using #config(ODIN_DYNAMIC_CRT, false)
at every use site?)
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.
The issue is that the libc is "dynamic" by default on other platforms. Shouldn't it be dynamic on windows by default as well?
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.
No. Don't try to compare Windows to other platforms, that is rarely a good idea, especially in this case. The problem is that Windows has different variants of libc, both dynamic and static, and they do conflict with each other (MD, MT, MDd, MTd, etc).
The one we picked in libc
is one of the most common, but it seems to conflict with raylib (and it's always raylib that people bring up) every single time.
The other issue is that people who do this, rarely actually need anything from raylib, but because they are used to C (or copying a tutorial written in C), they think using libc is what they should do, rather than other native Odin things.
This PR has been left for a long while because it's fundamentally a hard problem, and this solution is just a bad bodge. It doesn't really solve the fundamental problem that it is trying to solve.
I won't close the PR, but it's just not solved by doing this.
Opt-in flag to link the ucrt for
core:c/libc
viaucrt.lib
instead oflibucrt.lib
. This should be equivalent to switching from/MT
to/MD
.