builder: use cc enum in CcompilerOptions, fix cc detection, enable cc guessing without prod flag #21370

ttytm · 2024-04-28T21:59:57Z

The PR utilizes an enum for the CC instead of multiple boolean fields on the CcompilerOptions struct. Since only one CC can be used at a time, structuring things this way can make sense.

A nice side effect of the change was discovering a bug where the CC is not set when compiling V on macOS. This was fixed by enabling compiler guessing on macOS even when V is not compiled in prod mode

Example CI after swapping to use an enum and panicking on an unknown compiler:
- macos-12: https://github.com/ttytm/v/actions/runs/8870500885/job/24352354898
- macos-14: https://github.com/ttytm/v/actions/runs/8870500874/job/24352354543
To pass the sanitized workflow, a vpath in a step (which was enabled in a recently made change) was fixed. Additionally, a trigger was added for changes to this sanitized workflow.

Edit:
Refs. for development / review purposes

Commit no. 3 9b58466 contains the first full CI run after the initial changes from which further improvements were started.

ttytm · 2024-04-28T22:15:26Z

Checking the other fails that I couldn't spot before the PR:
If moving towards using an enum, should all supported compilers be part of the enum, or should there be a other enum field that is used when the cc is an unknown compiler?

JalonSolov · 2024-04-28T22:25:43Z

other makes more sense. Especially when the cc might be a cross-compiler, zig, or anything else that's C99 compatible.

JalonSolov · 2024-04-28T22:27:03Z

For example, you can also choose -cc g++, and it usually works fine.

In other words, there is no current way to know all possible compilers.

ttytm · 2024-04-28T22:37:40Z

Thanks for the input.

I let the current CI finish to be able to analyze the full run before making an update.

vlib/v/builder/cc.v

JalonSolov · 2024-04-29T02:41:21Z

vlib/v/builder/cc.v

+			cc_file_name.contains('clang') || ccoptions.guessed_compiler == 'clang' { .clang }
+			cc_file_name.contains('msvc') || ccoptions.guessed_compiler == 'msvc' { .msvc }
+			cc_file_name.contains('icc') || ccoptions.guessed_compiler == 'icc' { .icc }
+			else { panic('unknown C compiler `${cc_file_name}`') }


Thought you were going to have an other type?

That's why the commit message

improve info on unknown compiler (for now keep panic for CI, add .unknown to enum later)

I forced pushed, but the message was the same.

It helps to work with the errors for now instead of silencing them.

Thought of adding an unknown field rather than other then as it still can be a gcc or clang just not detected. other implies it is something else. Also an error message but not a panic when it's an unknown compiler.

Looks like we can pass now without panicking when keeping the panic. Keeping it would make it immediately clear when there is an unknown compiler and potentially prevent regressions and unknown behaviour. Also increase the likelihood of making us aware through reports when we can add a not yet supported compiler. If keeping the panic is not an option I'd follow through with what was stated in the comment above.

Also increase the likelihood of making us aware through reports when we can add a not yet supported compiler.

The likelyhood is imho that people experimenting with such compilers will just not report anything and skip V.

The only requirement that we do have, is that the C compiler should support C99. Not that it is named in a particular "supported" way. That is good, and it should stay that way.

…nown to enum later)

ttytm · 2024-04-29T02:57:18Z

Enabled guessing the compiler outside of prod. The reason that it was done only in prod was performance. Not tested yet but the gain through saving the redundant ccompiler_file_name.contains(...) checks and due to the improved code structure should relativize performance losses.

… `cc --version` to guess compiler

.github/workflows/sanitized_ci.yml

vlib/v/builder/cc.v

spytheman · 2024-04-29T10:04:36Z

vlib/v/builder/cc.v

-	if ccoptions.guessed_compiler == 'cc' && v.pref.is_prod {
-		// deliberately guessing only for -prod builds for performance reasons
-		ccversion := os.execute('cc --version')
-		if ccversion.exit_code == 0 {


The is_prod check is there for a reason. Restore it.

I do not like how you are doing refactorings (the replacement of the strings and several bool fields -> a single enum is good), mixed with functional changes in behavior (which is questionable) :-| .

Please do keep such PRs separate. One can be merged, and the other closed. Now, you have to do more work to separate them, and I to review it.

The is_prod check is there for a reason. Restore it.

If I'm not forbidden to tust the comment. The reason why it's there is performance.
But there is no performance regression with the update. Instead there is a benefit of several fixes.

Please read:

It fixes the compiler being detected. E.g. on macos in general. This was stated in the issue description:

screenshot macos

So before opening the PR I just enabled guessing outside prod on macos. After opening it, it revealed that there are similar scenarios on other platforms. I classified that detecting a compiler is of priority - even if there would be a performance regression:

screenshot freestanding

screenshot vsl/vtl test

screenshot freebsd

Another fix that was made, unrelated to enabling guessing without prod:

screenshot g++

So it is mainly a fix, not a refactor.

I know there is a lot of descriptions in the PR, I try to keep them simple and down to what is important to simplify reviewing. But doesn't seem like anyone was read before writing the review comments. The changes pointed out in the reviews were also pointed out in the descriptions / open discussions where joining on would allow to address them better. I see your point but on some code it's hard sometimes impossible to keep changes very simple or would take a lot of time and PRs so I try to find a middlegroud.

I do not like how you are doing refactorings (the replacement of the strings and several bool fields -> a single enum is good), mixed with functional changes in behavior (which is questionable) :-| .

As stated above it's more of a fix than a refactor. Using an enum had to be exclusive. And in the first stage a panic was added if the compiler is unknown. If the panic is kept was up for discussion. It helped to detect where a compiler is not known during the work on the fix.

All of those are panics, that I do insist should not be there at all.

The portability of V to new platforms and OSes, will be limited by having a panic for no good reason at all.
The failures that you claim to be fixed, are self inflicted, by the very panics that I do insist should not be there. The check for -prod is also related - without it, the restrictions to cc are practically non existent, and that facilitates bootstrapping on new platforms, where it can be anything.

Again, V requires a C99 compiler, not a specific named one, and that should continue to be so. It should not panic when it can not decide what the compiler is, it should just not add compiler specific flags.

Yes we don't need to dwell down so much on the panics. I made the experience often enough that it's good to turn something into an error that should be a notice or warning in the result, just to be able to better work with it during development.

I would update the panic like stated here: #21370 (comment). Would that make the PR a merge candidate?

Even an error is not appropriate imho, since it should not prevent the compilation and bootstrapping on a new system.

At best, it can be a notice.

Yes, looks like we agree. It would be just a print to stderr to make note of using an undetected compiler (as it's done with other notices in the builder/cc.v), things would continue as usual.

Updated. As - with the PRs changes - we didn't panicked with all our currently tested options, the notice should only become visible when there is an unknown compiler.

ttytm added 3 commits April 28, 2024 23:40

builder: use enum for cc

7a96149

fix setting cc on macos

a8e62e3

fix vpath in sanitized run

9b58466

improve cc determination, always guess

1047fd6

JalonSolov reviewed Apr 29, 2024

View reviewed changes

vlib/v/builder/cc.v Outdated Show resolved Hide resolved

JalonSolov reviewed Apr 29, 2024

View reviewed changes

improve info on unknown compiler (for now keep panic for CI, add .unk…

9e8b7cb

…nown to enum later)

ttytm force-pushed the builder/enum-cc branch from b828241 to b8c39d8 Compare April 29, 2024 02:49

ttytm marked this pull request as draft April 29, 2024 03:10

ttytm force-pushed the builder/enum-cc branch from b8c39d8 to d3a2b83 Compare April 29, 2024 03:16

further improve when compiler is cc

4279761

ttytm force-pushed the builder/enum-cc branch from d3a2b83 to 4279761 Compare April 29, 2024 03:18

cleanup (still panic if compiler would be unknown)

1a16b50

ttytm marked this pull request as ready for review April 29, 2024 03:44

add panic for edge case when ccompiler == cc and failing to execute…

e771063

… `cc --version` to guess compiler

ttytm force-pushed the builder/enum-cc branch from 936904e to e771063 Compare April 29, 2024 04:01

ttytm changed the title ~~builder: use enum for cc, enable compiler guessing without prod flag on macos~~ builder: use enum for cc, enable compiler guessing without prod flag Apr 29, 2024

ttytm changed the title ~~builder: use enum for cc, enable compiler guessing without prod flag~~ builder: use cc enum in CcompilerOptions, fix cc detection, enable cc guessing without prod flag Apr 29, 2024

spytheman reviewed Apr 29, 2024

View reviewed changes

.github/workflows/sanitized_ci.yml Outdated Show resolved Hide resolved

spytheman reviewed Apr 29, 2024

View reviewed changes

vlib/v/builder/cc.v Outdated Show resolved Hide resolved

spytheman reviewed Apr 29, 2024

View reviewed changes

vlib/v/builder/cc.v Outdated Show resolved Hide resolved

spytheman reviewed Apr 29, 2024

View reviewed changes

spytheman and others added 3 commits April 29, 2024 14:02

Update .github/workflows/sanitized_ci.yml

863af9b

remove panics on unknown C compiler

bce3165

fix typos

ed619f8

spytheman approved these changes Apr 29, 2024

View reviewed changes

spytheman merged commit ec59760 into vlang:master Apr 29, 2024
66 checks passed

ttytm deleted the builder/enum-cc branch April 29, 2024 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

builder: use cc enum in CcompilerOptions, fix cc detection, enable cc guessing without prod flag #21370

builder: use cc enum in CcompilerOptions, fix cc detection, enable cc guessing without prod flag #21370

ttytm commented Apr 28, 2024 •

edited

Loading

ttytm commented Apr 28, 2024

JalonSolov commented Apr 28, 2024

JalonSolov commented Apr 28, 2024

ttytm commented Apr 28, 2024

JalonSolov Apr 29, 2024

ttytm Apr 29, 2024 •

edited

Loading

ttytm Apr 29, 2024 •

edited

Loading

spytheman Apr 29, 2024

ttytm commented Apr 29, 2024 •

edited

Loading

spytheman Apr 29, 2024

spytheman Apr 29, 2024

ttytm Apr 29, 2024 •

edited

Loading

ttytm Apr 29, 2024

spytheman Apr 29, 2024

spytheman Apr 29, 2024

ttytm Apr 29, 2024

spytheman Apr 29, 2024

ttytm Apr 29, 2024 •

edited

Loading

ttytm Apr 29, 2024

builder: use cc enum in CcompilerOptions, fix cc detection, enable cc guessing without prod flag #21370

builder: use cc enum in CcompilerOptions, fix cc detection, enable cc guessing without prod flag #21370

Conversation

ttytm commented Apr 28, 2024 • edited Loading

ttytm commented Apr 28, 2024

JalonSolov commented Apr 28, 2024

JalonSolov commented Apr 28, 2024

ttytm commented Apr 28, 2024

Choose a reason for hiding this comment

ttytm Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

ttytm Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttytm commented Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttytm Apr 29, 2024 • 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

ttytm Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttytm commented Apr 28, 2024 •

edited

Loading

ttytm Apr 29, 2024 •

edited

Loading

ttytm Apr 29, 2024 •

edited

Loading

ttytm commented Apr 29, 2024 •

edited

Loading

ttytm Apr 29, 2024 •

edited

Loading

ttytm Apr 29, 2024 •

edited

Loading