[C][Client] Make custom CMAKE_C_FLAGS work

[imaami]

Fixes CMakeFiles.txt so that the usual way to define custom C compile flags works. After the change users can set the initial value of CMAKE_C_FLAGS on the command line. Project-defined flags no longer replace the entire variable content but are appended instead. (Project flags still take priority if they need to override custom flags.)

For example to build a local version with architecture-specific CPU flags:

cmake -B /tmp/build -S /usr/src/whatever -DCMAKE_C_FLAGS='.march=znver3 -mtune=znver3'

The first commit enables CMakeLists.txt to be (re-)generated for one of the three C clients. It's been disabled for quite a long time, and it doesn't look like there is an actual reason for it. But please let me know if there's breakage.

[wing328]

thanks for the PR

moving forward, please copy the C technical committee:

@zhemant (2018/11) @ityuhui (2019/12) @michelealbano (2020/03) @eafer (2024/12)

[imaami]

thanks for the PR

moving forward, please copy the C technical committee:

@zhemant (2018/11) @ityuhui (2019/12) @michelealbano (2020/03) @eafer (2024/12)

Thanks, will do.

[imaami]

Rebased, and also added a minor CMakeLists.txt fix to remove redundant messages during configuration.

I forgot to mention one useful thing that becomes possible when CMAKE_C_FLAGS can be set on the command line: the GitHub workflow can add extra warning flags to help catch difficult-to-spot mistakes.

[eafer]

@imaami I don't think you need to rebase your PRs so often, the C generator doesn't get many patches. It can be a problem because it makes it harder to tell if there were new changes after my reviews.

[eafer]

Everything looks good to me. I had noticed that one of the sample clients was ignoring my new build flags, but I didn't know why. Thank you for fixing that.

[eafer]

the GitHub workflow can add extra warning flags to help catch difficult-to-spot mistakes.

That sounds good. I'm thinking we could enable -Wall -Wextra for all builds, but only enable -Werror for the github ci, so that nothing breaks for the users.

[imaami]

@imaami I don't think you need to rebase your PRs so often, the C generator doesn't get many patches. It can be a problem because it makes it harder to tell if there were new changes after my reviews.

No problem. From now on I'll only rebase if I can see there are changes in master that conflict with my branch, or if they change the generated code. It's a habit of mine, I mostly use rebase merges personally so it tends to show in the workflow.

[imaami]

the GitHub workflow can add extra warning flags to help catch difficult-to-spot mistakes.

That sounds good. I'm thinking we could enable -Wall -Wextra for all builds, but only enable -Werror for the github ci, so that nothing breaks for the users.

I usually enable at least -Wall -Wextra -Wpedantic everywhere. On clang I use -Weverything -Wpedantic followed by a long list of individual -Wno-... flags to remove the genuinely stupid warnings.

-Werror is OK, but just by itself (without at least -Wall -Wextra) it doesn't do much. I'm not sure if it makes sense to use it as a replacement for other -W flags.

Side note: right now -Wall -Wextra -Werror would cause the C client builds to fail. On clang just -Werror with default warnings would, too.

[eafer]

-Werror is OK, but just by itself (without at least -Wall -Wextra) it doesn't do much. I'm not sure if it makes sense to use it as a replacement for other -W flags.

That's not what I meant. We need -Werror for the ci because it won't report warnings. We don't want the users to get -Werror because some compiler versions will have different warnings and break the build. Of course the warnings have to be enabled too.

[eafer]

Side note: right now -Wall -Wextra -Werror would cause the C client builds to fail. On clang just -Werror with default warnings would, too.

Yes, I guess that should be expected, there are too many issues for now.

[imaami]

-Werror is OK, but just by itself (without at least -Wall -Wextra) it doesn't do much. I'm not sure if it makes sense to use it as a replacement for other -W flags.

That's not what I meant. We need -Werror for the ci because it won't report warnings. We don't want the users to get -Werror because some compiler versions will have different warnings and break the build. Of course the warnings have to be enabled too.

OK, I see. There's one catch about using -Werror for CI visibility: you only get a trickle of warnings. You'll only see the next warning after fixing the previous one and rebuilding. It can get a bit frustrating.

With a little effort it's fairly easy to get both the full list of warnings and a CI failure: just log the console output during build without -Werror, then finally after every build is done grep the logs for warnings. If there are any, return with failure for the CI.

I've done a lot of bash scripting and have also implemented the above before so it's feasible if you ask me. I'm pretty sure I'll find time for it at some point if it sounds like a good option.

[eafer]

The ci is just to catch regressions after we fix all the warnings. I'm always worried about this because I've fixed a few regressions lately that would have been obvious if anybody had checked the warnings, but the ci hides them.

[imaami]

Does this PR still need changes?

[eafer]

It looks good to me.