-
Notifications
You must be signed in to change notification settings - Fork 63
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
Rework C compiler detection #531
base: master
Are you sure you want to change the base?
Conversation
I'm generally in favor. I'd be surprised if anyone ran this in a windows command shell expecting krml to pick up on msvc -- if there are such people, we'll hear about them, but I doubt it. |
Also, the bat script is strange, because most people would open up a VS command prompt then invoke krml (meaning the right includes would be set in the path). I think that's a remnant of an old era where krml was run from a cygwin prompt and was expected to source the right environment variables itself. |
Sounds good, thanks. Maybe then I can remove the script altogether and make
it uniform to the other flavors. I'll update today.
…On Mon, Feb 10, 2025, 12:01 Jonathan Protzenko ***@***.***> wrote:
Also, the bat script is strange, because most people would open up a VS
command prompt *then* invoke krml (meaning the right includes would be
set in the path). I think that's a remnant of an old era where krml was run
from a cygwin prompt and was expected to source the right environment
variables itself.
—
Reply to this email directly, view it on GitHub
<#531 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAAJ75O42XSX5H3MORVX3L2PEAQ3AVCNFSM6AAAAABW3GBFNKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNBZGEYDMOJTGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Updated. Just calling
but these are in fact not installed in my system, so maybe I got a partly broken MSVC. There also seems to be no (This was in WSL, same thing happens from a plain cmd shell.) Also added an override for the flavor detection and fixed a few quirks. If you want to merge this I'll clean up the history and mark ready. |
Yeah I think that's the reason d'être of the script -- it calls vcvarsall which sets the right environment variables to help cl find the includes (I think) |
You're right, somehow I got mixed up and couldn't find the varsall script in my installation, but I see it now and it does make cl work. However I don't have any of the variables that the script looked for ( I suppose calling this script and then running krml would work fine, but no idea how the environments would interact if krml is ran from a cygwin shell. Also apparently starting with Visual Studio 2017 the situation is different, the variables are not set and one needs to (somehow find and then) call a shell wrapper script. https://help.appveyor.com/discussions/kb/48-vs2017. So... a big mess. Any thoughts? Maybe one way around this is specially recognizing when cc equals |
Yeah now you're supposed to use vswhere for that purpose: https://github.com/microsoft/vswhere Maybe that can replace the old bat script? |
bd62f9d
to
eb642ae
Compare
Updated to special-case Also added a case in that script to call vswhere if available, and that seems to work locally. Separate PR since it's independent from this #535. |
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.
Ok looks good to me with the other script change. Thanks!
This patch makes karamel pick, by default, 'cc' from the path as the C compiler. This command can be overriden with the -cc option (taking the name of a program in the PATH, or a full path to an executable), or the CC environment variable (idem). Since karamel passes specific options to different compilers, there is now logic to detect the flavor of compiler that karamel is talking to (e.g. by looking at `cc --version`). The flavor can be overridden with -ccflavor. There is a special case when the compiler is exactly "msvc" (either via -cc or CC): this is not taken to be a command nor path, and we instead use the wrapper script exactly as before.
Updated, thanks! |
This patch makes karamel pick, by default, 'cc' from the path as the C compiler. This command can be overriden with the -cc option (taking the name of a program in the PATH, or a full path to an executable), or the CC environment variable (idem).
Since karamel passes specific options to different compilers, there is now logic to detect the flavor of compiler that karamel is talking to (e.g. by looking at
cc --version
).MISSING: MSVC autodetection, and fixing the wrapper script. I don't have enough experience using it to know if we can tweak this script so as to only point it to an executable.
Also I think I should add a way to override the flavor in case autodetection fails.
(If you feel this change is more trouble than it's worth I'm ok dropping it.)
Fixes #529