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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 11 commits into from
Apr 29, 2024
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7a96149
builder: use enum for cc
ttytm Apr 28, 2024
a8e62e3
fix setting cc on macos
ttytm Apr 28, 2024
9b58466
fix vpath in sanitized run
ttytm Apr 28, 2024
1047fd6
improve cc determination, always guess
ttytm Apr 29, 2024
9e8b7cb
improve info on unknown compiler (for now keep panic for CI, add .unk…
ttytm Apr 29, 2024
4279761
further improve when compiler is `cc`
ttytm Apr 29, 2024
1a16b50
cleanup (still panic if compiler would be unknown)
ttytm Apr 29, 2024
e771063
add panic for edge case when ccompiler == `cc` and failing to execute…
ttytm Apr 29, 2024
863af9b
Update .github/workflows/sanitized_ci.yml
spytheman Apr 29, 2024
bce3165
remove panics on unknown C compiler
ttytm Apr 29, 2024
ed619f8
fix typos
ttytm Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/sanitized_ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ on:
- 'vlib/v/markused/**.v'
- 'vlib/v/preludes/**.v'
- 'vlib/v/embed_file/**.v'
- '**/sanitized_ci.yml'
pull_request:
paths:
- '!**'
Expand All @@ -64,6 +65,7 @@ on:
- 'vlib/v/markused/**.v'
- 'vlib/v/preludes/**.v'
- 'vlib/v/embed_file/**.v'
- '**/sanitized_ci.yml'

concurrency:
group: ${{ github.workflow }}-${{ github.ref == 'refs/heads/master' && github.sha || github.ref }}
Expand Down Expand Up @@ -161,7 +163,7 @@ jobs:
.\make.bat -msvc
.\v.exe self
- name: Ensure code is well formatted
run: v test-cleancode
run: ./v test-cleancode
spytheman marked this conversation as resolved.
Show resolved Hide resolved
# - name: Install dependencies
# run: |
# .\v.exe setup-freetype
Expand Down
83 changes: 44 additions & 39 deletions vlib/v/builder/cc.v
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,18 +94,21 @@ fn (mut v Builder) show_cc(cmd string, response_file string, response_file_conte
}
}

enum CC {
tcc
gcc
icc
msvc
clang
}

struct CcompilerOptions {
mut:
guessed_compiler string
shared_postfix string // .so, .dll
//
//
debug_mode bool
is_cc_tcc bool
is_cc_gcc bool
is_cc_icc bool
is_cc_msvc bool
is_cc_clang bool
debug_mode bool
cc CC
//
env_cflags string // prepended *before* everything else
env_ldflags string // appended *after* everything else
Expand Down Expand Up @@ -175,31 +178,33 @@ fn (mut v Builder) setup_ccompiler_options(ccompiler string) {
}
ccoptions.debug_mode = v.pref.is_debug
ccoptions.guessed_compiler = v.pref.ccompiler
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 {
Comment on lines -178 to -181
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@ttytm ttytm Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

    Screenshot_20240429_122657

    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_20240429_122712

    screenshot vsl/vtl test

    Screenshot_20240429_122751

    screenshot freebsd

    Screenshot_20240429_122831

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

screenshot g++

Screenshot_20240429_122817

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@ttytm ttytm Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ccversion.output.contains('This is free software;')
&& ccversion.output.contains('Free Software Foundation, Inc.') {
ccoptions.guessed_compiler = 'gcc'
}
if ccversion.output.contains('clang version ') {
ccoptions.guessed_compiler = 'clang'
if ccoptions.guessed_compiler == 'cc' {
if cc_ver := os.execute_opt('cc --version') {
ccoptions.cc = match true {
// Also covers `g++`, `g++-9`, `g++-11` etc.
cc_ver.output.replace('\n', '').contains('Free Software Foundation, Inc.This is free software;') { .gcc }
cc_ver.output.contains('clang version ') { .clang }
else { panic('failed to detect C compiler from version info `${cc_ver.output}`') }
ttytm marked this conversation as resolved.
Show resolved Hide resolved
}
} else {
panic('unknown C compiler')
}
} else {
cc_file_name := os.file_name(ccompiler)
ccoptions.cc = match true {
// vfmt off
cc_file_name.contains('tcc') || ccoptions.guessed_compiler == 'tcc' { .tcc }
cc_file_name.contains('gcc') || cc_file_name.contains('g++') || ccoptions.guessed_compiler == 'gcc' { .gcc }
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}`') }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought you were going to have an other type?

Copy link
Member Author

@ttytm ttytm Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@ttytm ttytm Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ttytm marked this conversation as resolved.
Show resolved Hide resolved
// vfmt on
}
}
ccompiler_file_name := os.file_name(ccompiler)
ccoptions.is_cc_tcc = ccompiler_file_name.contains('tcc') || ccoptions.guessed_compiler == 'tcc'
ccoptions.is_cc_gcc = ccompiler_file_name.contains('gcc') || ccoptions.guessed_compiler == 'gcc'
ccoptions.is_cc_icc = ccompiler_file_name.contains('icc') || ccoptions.guessed_compiler == 'icc'
ccoptions.is_cc_msvc = ccompiler_file_name.contains('msvc')
|| ccoptions.guessed_compiler == 'msvc'
ccoptions.is_cc_clang = ccompiler_file_name.contains('clang')
|| ccoptions.guessed_compiler == 'clang'

// Add -fwrapv to handle UB overflows
if (ccoptions.is_cc_gcc || ccoptions.is_cc_clang || ccoptions.is_cc_tcc)
&& v.pref.os in [.macos, .linux, .windows] {
if ccoptions.cc in [.gcc, .clang, .tcc] && v.pref.os in [.macos, .linux, .windows] {
ccoptions.args << '-fwrapv'
}

Expand All @@ -208,7 +213,7 @@ fn (mut v Builder) setup_ccompiler_options(ccompiler string) {
ccoptions.args << '-fpermissive'
ccoptions.args << '-w'
}
if ccoptions.is_cc_clang {
if ccoptions.cc == .clang {
if ccoptions.debug_mode {
debug_options = ['-g', '-O0']
}
Expand All @@ -227,7 +232,7 @@ fn (mut v Builder) setup_ccompiler_options(ccompiler string) {
'-Wno-int-to-void-pointer-cast',
]
}
if ccoptions.is_cc_gcc {
if ccoptions.cc == .gcc {
if ccoptions.debug_mode {
debug_options = ['-g']
if user_darwin_version > 9 {
Expand All @@ -236,7 +241,7 @@ fn (mut v Builder) setup_ccompiler_options(ccompiler string) {
}
optimization_options = ['-O3', '-flto']
}
if ccoptions.is_cc_icc {
if ccoptions.cc == .icc {
if ccoptions.debug_mode {
debug_options = ['-g']
if user_darwin_version > 9 {
Expand All @@ -251,7 +256,7 @@ fn (mut v Builder) setup_ccompiler_options(ccompiler string) {
}
if v.pref.is_prod {
// don't warn for vlib tests
if ccoptions.is_cc_tcc && !(v.parsed_files.len > 0
if ccoptions.cc == .tcc && !(v.parsed_files.len > 0
&& v.parsed_files.last().path.contains('vlib')) {
eprintln('Note: tcc is not recommended for -prod builds')
}
Expand Down Expand Up @@ -298,15 +303,15 @@ fn (mut v Builder) setup_ccompiler_options(ccompiler string) {
ccoptions.args << '-Wl,--no-entry'
}
if ccoptions.debug_mode && builder.current_os != 'windows' && v.pref.build_mode != .build_module {
if builder.current_os == 'macos' && !ccoptions.is_cc_tcc {
if ccoptions.cc != .tcc && builder.current_os == 'macos' {
ccoptions.linker_flags << '-Wl,-export_dynamic' // clang for mac needs export_dynamic instead of -rdynamic
} else {
ccoptions.linker_flags << '-rdynamic' // needed for nicer symbolic backtraces
}
}
if v.pref.os == .freebsd {
// Needed for -usecache on FreeBSD 13, otherwise we get `ld: error: duplicate symbol: _const_math__bits__de_bruijn32` errors there
if !ccoptions.is_cc_tcc {
if ccoptions.cc != .tcc {
ccoptions.linker_flags << '-Wl,--allow-multiple-definition'
} else {
// tcc needs this, otherwise it fails to compile the runetype.h system header with:
Expand All @@ -318,7 +323,7 @@ fn (mut v Builder) setup_ccompiler_options(ccompiler string) {
if ccompiler != 'msvc' && v.pref.os != .freebsd {
ccoptions.wargs << '-Werror=implicit-function-declaration'
}
if ccoptions.is_cc_tcc {
if ccoptions.cc == .tcc {
// tcc 806b3f98 needs this flag too:
ccoptions.wargs << '-Wno-write-strings'
}
Expand All @@ -333,7 +338,7 @@ fn (mut v Builder) setup_ccompiler_options(ccompiler string) {

// macOS code can include objective C TODO remove once objective C is replaced with C
if v.pref.os in [.macos, .ios] {
if !ccoptions.is_cc_tcc && !user_darwin_ppc && !v.pref.is_bare && ccompiler != 'musl-gcc' {
if ccoptions.cc != .tcc && !user_darwin_ppc && !v.pref.is_bare && ccompiler != 'musl-gcc' {
ccoptions.source_args << '-x objective-c'
}
}
Expand Down Expand Up @@ -378,14 +383,14 @@ fn (mut v Builder) setup_ccompiler_options(ccompiler string) {
ccoptions.pre_args << others
ccoptions.linker_flags << libs
if v.pref.use_cache && v.pref.build_mode != .build_module {
if !ccoptions.is_cc_tcc {
if ccoptions.cc != .tcc {
$if linux {
ccoptions.linker_flags << '-Xlinker -z'
ccoptions.linker_flags << '-Xlinker muldefs'
}
}
}
if ccoptions.is_cc_tcc && 'no_backtrace' !in v.pref.compile_defines {
if ccoptions.cc == .tcc && 'no_backtrace' !in v.pref.compile_defines {
ccoptions.post_args << '-bt25'
}
// Without these libs compilation will fail on Linux
Expand Down Expand Up @@ -426,7 +431,7 @@ fn (v &Builder) all_args(ccoptions CcompilerOptions) []string {
// /volatile:ms - there seems to be no equivalent,
// normally msvc should use /volatile:iso
// but it could have an impact on vinix if it is created with msvc.
if !ccoptions.is_cc_msvc {
if ccoptions.cc != .msvc {
if v.pref.os != .wasm32_emscripten {
all << '-Wl,-stack=16777216'
}
Expand Down Expand Up @@ -625,7 +630,7 @@ pub fn (mut v Builder) cc() {
}
}
$if windows {
if v.ccoptions.is_cc_tcc {
if v.ccoptions.cc == .tcc {
def_name := v.pref.out_name[0..v.pref.out_name.len - 4]
v.pref.cleanup_files << '${def_name}.def'
}
Expand Down
Loading