Support reflecting CXXFLAGS and LDFLAGS environment variables

[onion108]

Added support for environment variables CXXFLAGS and LDFLAGS while generating makefile.

See #923.

[ken-matsui]

Why would you prefer to do this instead of my idea?

[onion108]

Firstly, the most important reason is, as far as I know, many build systems that are widely used made use of those environment variables (autoconf, Makefile, CMake etc.). I think it will be less confusing to make the behavior consistent as those build system. (Even cargo respects RUSTFLAGS)

Also sometimes I would like to temporarily change the flags frequently to quickly see the result and adjust my flags in the config after I'm confident about the result. I personally prefer setting the environment variables to edit the config file over and over again because it's convenient to just add an environment variable in the command line, and I don't need to repeatly edit and save a file just for something experimental (in my personal opinion).

Another reason (may be too personal) is that, if I downloaded a project that I don't own and I want to build it with my preferred toolchain, for instance I want to build with Homebrew ruby instead of the default ruby provided by macOS (but the project's config doesn't contain flags for Homebrew ruby), it's just convenient to just set an environment variable instead of editing config file, because if I changed the project config file and next time I want to upgrade the repository, I still need to revert the change, pull the repository and change it again. It may fail the compilation or even have different behavior after the compilation but since those environment variables were set by me, it means I know the risks and I'll take them. (And actually I set it globally, in real life)

By the way, (not very seriously though) we supported CXX environment variable so why not support others as well?

To conclude I think the most important reason is to keep the behavior consistent as build systems that are widely used, and it will be also super convenient for some users to quickly edit the flags just for experimental uses (especially command-line nerds like me, yeah). About the potential risks it may bring, like compilation error or different behavior etc., since the system won't set those environment variables automatically (and it means they are set only by user) I think user would and should take the risks, and once user set the environment variable (even globally) they usually knows what they're doing (and hoping the build system uses those variables).

[onion108]

P.S.: I do think your idea is great and should be used on most projects, but I think the support of environment flags still should be implemented as well for the consistency with other widely-used build systems and some advanced user may require it to work

[ken-matsui]

Thanks for the clarification, I'll review your PR when I get a chance. Please also make the PR title clearer.

[ken-matsui]

Thank you for working on this. This is a good start; I left some comments. Also, please address the clang-tidy error.

[ken-matsui]

Overall, this looks great. Thanks for your work. Let's tweak more details.

[onion108]

All review comments are resolved in the latest commit.

By the way I enabled workflow on the fork repo so I could run poac tidy and see the results to prevent me from making dumb mistakes. For some strange reasons (that I don't know) poac tidy's output in the actions is quite different from my machine (clang-tidy 19). My tidy seems telling me about almost nothing about my mistakes (like making const when possible), but always complains about circular header file dependency in Git2 library downloaded via Homebrew (which doesn't exists in the workflow output).

Another thing about tidy is after I committed my latest change, poac tidy (on the Github Workflow, hence the correct one) told me that in my test the code

assertEq(argsEscapeMixed.size(), 5);
                                ~^~

is a magic constant and tell me to consider replacing it with a named constant (but tbh we really need a named constant like ARGS_ESCAPE_MIXED_SIZE_EXPECTED just to check if the size is correct ...? I'm not quite sure about that but definitely it will not pass the clang-tidy action if I don't do so).

After digging deeper in the project I found a file Algos.cc that I think may be better for parseEnvFlags and its tests (and perhaps getEnvFlags as well?) to be placed in. I may consider doing in the next commit (after checking the code itself is fine and no dumb mistakes or naming mistakes or such), but I'm not quite sure if I'm going to move both parseEnvFlags and getEnvFlags, making parseEnvFlags internal to Algos.cc or just move parseEnvFlags to that file (and declare it in the header file as well).

[ken-matsui]

All review comments are resolved in the latest commit.

Thanks!

By the way I enabled workflow on the fork repo so I could run poac tidy and see the results to prevent me from making dumb mistakes. For some strange reasons (that I don't know) poac tidy's output in the actions is quite different from my machine (clang-tidy 19). My tidy seems telling me about almost nothing about my mistakes (like making const when possible), but always complains about circular header file dependency in Git2 library downloaded via Homebrew (which doesn't exists in the workflow output).

This problem was due to the previous poac tidy implementation, and I fixed it. Currently, the fix is available only on the main. GitHub Actions one is always using the source code from the commit that triggered the action, which always contains my fix anyway, so that's why we see differences.

Another thing about tidy is after I committed my latest change, poac tidy (on the Github Workflow, hence the correct one) told me that in my test the code

assertEq(argsEscapeMixed.size(), 5);
                                ~^~

is a magic constant and tell me to consider replacing it with a named constant (but tbh we really need a named constant like ARGS_ESCAPE_MIXED_SIZE_EXPECTED just to check if the size is correct ...? I'm not quite sure about that but definitely it will not pass the clang-tidy action if I don't do so).

Using a named constant here makes no sense, so let's use NOLINT (or NOLINTNEXTLINE depending on the code elegance). See here for more info.

After digging deeper in the project I found a file Algos.cc that I think may be better for parseEnvFlags and its tests (and perhaps getEnvFlags as well?) to be placed in. I may consider doing in the next commit (after checking the code itself is fine and no dumb mistakes or naming mistakes or such), but I'm not quite sure if I'm going to move both parseEnvFlags and getEnvFlags, making parseEnvFlags internal to Algos.cc or just move parseEnvFlags to that file (and declare it in the header file as well).

Currently, they are used only by BuildConfig.cc so it makes sense to put them here. There's no compelling reason to make them project public APIs. Thanks for the suggestion anyway :)

[onion108]

Thanks for your reply. Added NOLINTNEXTLINE for *-magic-numbers before size checks and checked Github Actions. All the tests on the Github Actions passed now.

[ken-matsui]

Thanks, will have a look when I get the time.

[ken-matsui]

Looks great. After tweaking small things, ok for main. Thanks.

[ken-matsui]

Thanks for your time to work on this!