-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
Add parser for MSVC /sourceDependencies output #1812
Conversation
What's the minimum language standard that Ninja supports? I didn't see anything documented in the coding style, but the Linux and macOS builds are failing because simdjson requires C++11 or greater. |
This comment was marked as abuse.
This comment was marked as abuse.
I switched from simdjson to RapidJSON to be C++98 compatible. |
42b765d
to
2e95e70
Compare
This comment was marked as abuse.
This comment was marked as abuse.
Thanks for the PR! Should this be Windows-only? This way we wouldn't include the big JSON dependency on the other platforms (or is there a use-case I'm missing?). |
Yes, I can make this Windows-only. The new format is only supported by MSVC (as far as I know). |
@jhasse a few questions about including RapidJSON:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
configure.py
deprecated?
No, CMake support has just been introduced. But it probably will be in the future.
Including the source code is a no-go. But you could - as this PR is Windows-only and our CMake version requirement is a lower entry-barrier there - use CMake to manage the RapidJSON dependency (see https://github.com/ninja-build/ninja/pull/1562/files#diff-af3b638bc2a3e6c650974192a53c7291R99 for an example). We could then either remove configure.py support for Windows or msvc_source_dependencies support when building with configure.py.
9f9c8db
to
924b609
Compare
@jhasse Do you have any other concerns or is this good to merge? I can squash commits to keep history clean. I removed the UTF-8 to ANSI conversion function, so this PR no longer has a dependency on #1671. It won't correctly handle paths with non-ASCII characters, but that's a pre-existing issue with Ninja on Windows if your console codepage doesn't match the encoding of the build.ninja file. |
Yes, please squash the commits. Before merging this needs further testing and Ninja 1.10.1 needs to be released. Also I won't merge the Microsoft copyright header, someone else needs to do that. |
a614e1f
to
6415ffe
Compare
Thanks @jhasse, I've squashed the commits. By "further testing" do you mean adding additional automated tests in this PR, or manually testing on a variety real-world systems? Given that you are unable to merge the Microsoft copyright header, who else is able to merge PRs to the Ninja project? |
This comment was marked as abuse.
This comment was marked as abuse.
The latter ;) The unit tests are great, very good work. |
closing in favor of #2200 |
This adds depfile support for the new
/sourceDependencies
format coming in MSVC in Visual Studio 16.7 and currently available in public preview. #1806 has more discussion about this flag./sourceDependencies
outputs a UTF-8 encoded JSON file with information about the headers used during compilation. With the changes in this PR, Ninja rules that specifydeps = msvc_source_dependencies
will consume a JSON file specified bydepfile
to compute dependency information.Within Ninja, RapidJSON is used to parse the JSON file and extract paths. Because Ninja on Windows uses the ANSI APIs instead of the Unicode APIs, we also have to convert the paths from UTF-8 to the current code page using a combination of MultiByteToWideChar and WideCharToMultiByte. This conversion will only happen on Windows and will partially address #1766.