Skip to content
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

Closed

Conversation

benmcmorran
Copy link

@benmcmorran benmcmorran commented Jul 15, 2020

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 specify deps = msvc_source_dependencies will consume a JSON file specified by depfile 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.

doc/manual.asciidoc Outdated Show resolved Hide resolved
@benmcmorran
Copy link
Author

benmcmorran commented Jul 15, 2020

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.

@jonesmz

This comment was marked as abuse.

@benmcmorran
Copy link
Author

I switched from simdjson to RapidJSON to be C++98 compatible.

@jonesmz

This comment was marked as abuse.

configure.py Outdated Show resolved Hide resolved
configure.py Outdated Show resolved Hide resolved
configure.py Outdated Show resolved Hide resolved
src/build.cc Outdated Show resolved Hide resolved
src/rapidjson/allocators.h Outdated Show resolved Hide resolved
src/util.cc Outdated Show resolved Hide resolved
@jhasse
Copy link
Collaborator

jhasse commented Jul 16, 2020

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?).

@benmcmorran
Copy link
Author

Yes, I can make this Windows-only. The new format is only supported by MSVC (as far as I know).

@benmcmorran
Copy link
Author

@jhasse a few questions about including RapidJSON:

  • Is configure.py deprecated?
  • If no, are you okay with directly including the RapidJSON source code in the repo like this PR is doing, or should I modify configure.py to have some crude dependency management system?
  • If yes, can I remove configure.py and just use CMake to manage the RapidJSON dependency? This avoids having RapidJSON checked in.

Copy link
Collaborator

@jhasse jhasse left a 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.

src/build.cc Outdated Show resolved Hide resolved
src/build.cc Outdated Show resolved Hide resolved
src/build.cc Outdated Show resolved Hide resolved
src/clsourcedependencies_parser.cc Outdated Show resolved Hide resolved
doc/manual.asciidoc Outdated Show resolved Hide resolved
src/clsourcedependencies_parser.cc Outdated Show resolved Hide resolved
@benmcmorran
Copy link
Author

@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.

@jhasse
Copy link
Collaborator

jhasse commented Jul 28, 2020

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.

@benmcmorran
Copy link
Author

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?

@jonesmz

This comment was marked as abuse.

@jhasse
Copy link
Collaborator

jhasse commented Jul 29, 2020

By "further testing" do you mean adding additional automated tests in this PR, or manually testing on a variety real-world systems?

The latter ;) The unit tests are great, very good work.

@jhasse
Copy link
Collaborator

jhasse commented Apr 9, 2024

closing in favor of #2200

@jhasse jhasse closed this Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants