move /showIncludes from always_args to ninja_compile_args#15960
Conversation
| commands = self.generate_basic_compiler_args(target, compiler) | ||
| # /showIncludes is needed for build dependency tracking in Ninja | ||
| # See: https://ninja-build.org/manual.html#_deps | ||
| if compiler.id in {'msvc', 'clang-cl', 'intel-cl'}: |
There was a problem hiding this comment.
Since you are removing the flag from VisualStudioLikeCompiler, please use isinstance(compiler, VisualStudioLikeCompiler).
There was a problem hiding this comment.
Thanks, that's much better.
There was a problem hiding this comment.
Unfortunately that means unconditionally importing additional modules on unix in order to check identity, which is something we mostly switched away from in favor of compiler.id equality comparisons out of concern for performance.
|
We have lots of operations in the Compiler classes that exist for only one backend. What issue are you solving by moving this to the ninja backend instead of being a property of the compiler? |
Short version: Compiles take 40% longer than they should with backend=vs. Long version: See #15959 |
|
I'd prefer if we added a new method to the Compiler class for that, implemented it as |
|
Oh indeed that's the better implementation! Sorry for missing the obvious |
The /showIncludes flag is required by ninja, but not by any of the other backends. Since this is a ninja-specific requirement, it makes sense to separate it from the rest of the always_args. Fixes a compile performance regression with the vs backend when using Visual Studio 2026 version 18.7 Fixes mesonbuild#15959
peterh
changed the title
| """Where is the libdir for the current machine located""" | ||
| raise EnvironmentException(f'{self.get_id()} does not support Rust target libdir') | ||
|
|
||
| def get_ninja_compile_args(self) -> T.List[str]: |
There was a problem hiding this comment.
I think it makes more sense to name this something like get_show_dep_args(), with a docstring like:
Arguments for printing depfile information in MSVC compatible format.
Only the base class needs the docstring.
There was a problem hiding this comment.
I respectfully disagree. That's what this function does, but it's not why this function does what it does.
If we were adding something to support a b_include_list option and there was a gcc version that returned ['-MD'], I would agree with you. But in this case, we're returning a list of arguments that ninja needs for this backend to work properly. We're not returning a flag that shows the end user the include files. (Well, we are, but that's not why we're returning that flag).
All that said, it's your project, so I'll change the name to whatever you like.
There was a problem hiding this comment.
I agree with your high level explanation but the function does not actually have anything to do with ninja!
It is a "not_vsbackend_compile_args" and should be used on any backend that isn't visual studio, surely. It just so happens that the only such backend today is Ninja, but if we ever added another one then surely we would want to have showincludes in use.
4 participants
The /showIncludes flag is required by ninja, but not by any of the other backends. Since this is a ninja-specific requirement, it makes sense to include it with the ninja backend.
Fixes a compile performance regression with the vs backend when using Visual Studio 2026 version 18.7
Fixes #15959