meson
meson copied to clipboard
Add compiler.get_crt() method
With MSVC, we sometimes need to know which runtime is used, to deduce the right dependency name to link with. get_option('b_vscrt') is not sufficient because the crt may be deduced from the buildtype. This new method returns the crt that the compiler will effectively use.
Fixes #8493
This should probably raise a MesonException or appropriate child exception when the compiler doesn't have a Microsoft CRT, to prevent people from assuming its meaning. It should only be used inside of if/else checks for the compiler / OS id.
I'm not sure what is the best way to handle this. I added a call to get_crt_compile_args to get the exception from the base Compiler class, but CLikeCompiler implement this function by returning an empty array, therefore all c-like compilers will return 'none' if b_vscrt is not set to a specific value.
@eli-schwartz Do you still have concerns about this PR?
Why does this add 22 new tests?
What happens on non-Windows platforms?
Why does this add 22 new tests?
Because it tests combinations of b_vscrt and buildtype.
What happens on non-Windows platforms?
It depends whether the compiler implements get_crt_compile_args or not. The default is to raise EnvironmentException. Otherwise, it returns the effective value of b_vscrt, or 'none' if there is not such value.
Why does this add 22 new tests?
Because it tests combinations of
b_vscrtandbuildtype.
Let me rephrase that.
Why does this add 22 new tests, this time with an answer that provides insight into the value of the 22 new tests rather than stating the mechanical designation of the tests?
It depends whether the compiler implements
get_crt_compile_argsor not
Can you give examples? The code tells me that it depends on that method -- the reason I'm asking, is because I'm not at my computer and it's hard to check right now.
But also because good commit messages tell not just what, but why, and in general all non-obvious details such as ramifications. It's helpful for exactly such cases like this.
For the test, I'm testing that each buildtype gives the right crt when b_vscrt is from_buildtype or static_from_buildtype (5 build types (custom does not apply) x 2 b_vscrt = 10 combinations), and that explicitly set b_vscrt is returned as well (4 b_vscrt, tested with 2 buildtypes = 8 combinations) and 'none' b_vscrt with 6 buildtype, for a total of 24 tests (I don't know where you took 22 from...)
| debug | debugoptimized | release | minsize | plain | custom | |
|---|---|---|---|---|---|---|
| mt | x | x | ||||
| md | x | x | ||||
| mtd | x | x | ||||
| mdd | x | x | ||||
| none | x | x | x | x | x | x |
| from_buildtype | x | x | x | x | x | |
| static_from_buildtype | x | x | x | x | x |
I don't want to put logic into the interpreter function. It must be the compiler that determines whether it can return a crt or not. I changed the logic a little bit:
- if the compiler
get_crt_compile_argsfunction raises, I do not catch the exception, and meson will fail - if the compiler
get_crt_compile_argsreturns nothing, I return 'none' - otherwise, I return what
compiler.get_crt_valreturns
For the test, I'm testing that each buildtype gives the right crt when [...]
... :eyes:
Perhaps it would help you understand my apprehension here if I pointed out that generally, adding a single feature doesn't necessitate adding two dozen unittests to test that it works correctly -- and in particular:
- the most expensive thing about a test is generally the overhead of running a meson project cycle, not the actual tested content itself
- Windows is already the most expensive and time consuming test platform by an order of magnitude, so adding two dozen new tests to Windows specifically seems like going in exactly the opposite direction of what we want
And you're still just explaining what the tests are coded to do, not why it's important to do them (so many times).
I would like to see comparable test coverage implemented in much less test runtime. I suspect it can be done.
I would like to see comparable test coverage implemented in much less test runtime. I suspect it can be done.
I agree... But I guess the only way would be to do it as a unit test. I will try this.
@xclaesse I saw it is you who added the ml compiler. Could you please check the first commit? I think we must return the proper /md, etc. switch instead of an empty array for that compiler, but I'm unsure since I don't know anything about it...
@eli-schwartz I finally refactored the get_crt_compile_args() and get_crt_link_args() stuff. It allowed to simplify and streamline the get_crt() interpreter function. I also rewrote the test because you were right there were not mush added value testing the combinations of buildtype and b_vscrt options. It now rather tests that the interpreter function works, and that it raises an error on unsupported compilers.
The concept of C runtime library matters on e.g. Linux too, where we have to choose between libgcc and compiler-rt.
I don't think you must wire that up now, but it might be nice to take that into account wrt TODOs and the documentation of the new option at least.
That said, the name makes it sound like it could already work on Linux (it's not a Windows-specific name) and people might reasonably expect it to do as it says...
The good news is that it's supposed to error out if unsupported (there's a reason I asked for that) which gives us time to actually wire it up without unduly confusing people.