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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

mtzguido
Copy link
Member

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

@mtzguido mtzguido marked this pull request as draft February 10, 2025 18:11
@msprotz
Copy link
Contributor

msprotz commented Feb 10, 2025

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.

@msprotz
Copy link
Contributor

msprotz commented Feb 10, 2025

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.

@mtzguido
Copy link
Member Author

mtzguido commented Feb 10, 2025 via email

@mtzguido
Copy link
Member Author

Updated. Just calling cl.exe seems to kind of work... except that it doesn't find standard headers:

$ CC=cl.exe make test
[...]
../_build/default/src/Karamel.exe -fstar fstar.exe  -warn-error @4 -verbose -skip-makefiles  -tmpdir .output/Abbrev.out -no-prefix Abbrev \
  -o .output/Abbrev.exe [...] -bundle Abbrev=WindowsHack,\*
✔ [Monomorphization] ⏱️ 58ms
✔ [Inlining] ⏱️ 7ms
✔ [Pattern matches compilation] ⏱️ 7ms
✔ [Structs + Simplify 2] ⏱️ 8ms
✔ [Drop] ⏱️ <1ms
✔ [AstToCStar] ⏱️ 2ms
✔ [CStarToC] ⏱️ <1ms
⚙ KaRaMeL auto-detecting tools. Here's what we found:
readlink is: readlink
KaRaMeL called via: ../_build/default/src/Karamel.exe
KaRaMeL home is: /home/guido/r/karamel
✔ [PrettyPrinting] ⏱️ 4ms
KaRaMeL: wrote out .c files for Abbrev
KaRaMeL: wrote out .h files for Abbrev
⚙ KaRaMeL will drive the C compiler. Here's what we found:
cc is: cl.exe (= /mnt/c/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/14.42.34433/bin/Hostx64/x64/cl.exe)
cc flavor is: msvc
cl.exe options are: -I /home/guido/r/karamel/krmllib/dist/minimal -I /home/guido/r/karamel/krmllib -I /home/guido/r/karamel/include -I .output/Abbrev.out
⚡ Generating object files
cl.exe -I /home/guido/r/karamel/krmllib/dist/minimal -I /home/guido/r/karamel/krmllib -I /home/guido/r/karamel/include -I .output/Abbrev.out -c .output/Abbrev.out/Abbrev.c -o .output/Abbrev.out/Abbrev.obj
✘ [CC,.output/Abbrev.out/Abbrev.c]
Microsoft (R) C/C++ Optimizing Compiler Version 19.42.34436 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

cl : Command line warning D9035 : option 'o' has been deprecated and will be removed in a future release

Abbrev.c
\\wsl.localhost\Ubuntu\home\guido\r\karamel\include\krml/internal/target.h(7): fatal error C1083: Cannot open include file: 'assert.h': No such file or directory

Warning 3: run_or_warn: the following command failed:
cl.exe -I /home/guido/r/karamel/krmllib/dist/minimal -I /home/guido/r/karamel/krmllib -I /home/guido/r/karamel/include -I .output/Abbrev.out -c .output/Abbrev.out/Abbrev.c -o .output/Abbrev.out/Abbrev.obj
Warning 3 is fatal, exiting.
make[1]: *** [Makefile:131: .output/Abbrev.exe] Error 255
make[1]: Leaving directory '/home/guido/r/karamel/test'
make: *** [Makefile:57: test] Error 2

but these are in fact not installed in my system, so maybe I got a partly broken MSVC. There also seems to be no vcvarsall script for the 2022 edition I installed, maybe the need for that is gone? (Also note the warning about /o surprisingly being deprecated!?) Would be useful if someone has a working MSVC and wants to test.. but maybe no one is doing that.

(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.

@msprotz
Copy link
Contributor

msprotz commented Feb 11, 2025

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)

@mtzguido
Copy link
Member Author

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 (%VS150COMNTOOLS%, etc) set.

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 msvc, and just using all the old logic there. I don't think it will work in VS 2017 and upwards though.

@msprotz
Copy link
Contributor

msprotz commented Feb 11, 2025

Yeah now you're supposed to use vswhere for that purpose: https://github.com/microsoft/vswhere

Maybe that can replace the old bat script?

@mtzguido
Copy link
Member Author

Updated to special-case -cc msvc (or CC=msvc) and call the wrapper script if so.

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.

Copy link
Contributor

@msprotz msprotz left a 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.
@mtzguido mtzguido changed the title WIP: Rework compiler autodetection Rework C compiler detection Feb 12, 2025
@mtzguido mtzguido marked this pull request as ready for review February 12, 2025 21:45
@mtzguido
Copy link
Member Author

Updated, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cc autodetection?
2 participants