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

Stop -t msvc -o from lowercasing paths from /showIncludes output. #589

Merged
merged 1 commit into from
May 29, 2013

Conversation

nico
Copy link
Collaborator

@nico nico commented May 29, 2013

/showIncludes prints absolute paths. If a source file says
#include <WiNdOwS.h>, /showIncludes will use that spelling in
its output for the basename and use the on-disk cache for the
directory names in the rest for its output.

This makes the .d files created by -t msvc -o consistent with the
.d files written by gcc and clang.
�Before this change, -t msvc -o would convert this output to
lower case. This is a problem if a build step produces a header file
with mixed case, such as "RuntimeFeatures.h". Due to the lowercasing,
the .d file would contain "runtimefeatures.h", while the build step
will create "RuntimeFeatures.h". Due to the case difference, ninja
would not realize that regeneration of the .h file would require
a rebuild of all source files having the header in the .d file.
(On the next build, ninja would rebuild them since stat()ing is not
case-sensitive on Windows.) One possible fix for this is to make sure
that generators always write generated header files in lower case too,
but on Mac gcc doesn't do lower-casing and .d files end up with the
case-as-written, so generators would have to be different on Mac and
Windows, which is undesirable.

If case-insensitve path comparisons are useful, they should be done
somewhere else (e.g. in CanonicalizePath()) where they affect both
paths read from .d files and paths read from .ninja files. This should
then be controlled by a top-level variable.

This patch changes behavior, but it only has an effect on generated
header files, which aren't common, and it only affects -t msvc, which
is still marked as experimental. (cmake doesn't use it yet.)

(If a file has both #include <windows.h> and <Windows.h>, this
will now take 2 stat() calls instead of just one, but that should
have a negligible cost.)

/showIncludes prints absolute paths. If a source file says
`#include <WiNdOwS.h>`, /showIncludes will use that spelling in
its output for the basename and use the on-disk cache for the
directory names in the rest for its output.

This makes the .d files created by `-t msvc -o` consistent with the
.d files written by gcc and clang.
�Before this change, `-t msvc -o` would convert this output to
lower case. This is a problem if a build step produces a header file
with mixed case, such as "RuntimeFeatures.h". Due to the lowercasing,
the .d file would contain "runtimefeatures.h", while the build step
will create "RuntimeFeatures.h". Due to the case difference, ninja
would not realize that regeneration of the .h file would require
a rebuild of all source files having the header in the .d file.
(On the next build, ninja would rebuild them since stat()ing is not
case-sensitive on Windows.) One possible fix for this is to make sure
that generators always write generated header files in lower case too,
but on Mac gcc doesn't do lower-casing and .d files end up with the
case-as-written, so generators would have to be different on Mac and
Windows, which is undesirable.

If case-insensitve path comparisons are useful, they should be done
somewhere else (e.g. in CanonicalizePath()) where they affect both
paths read from .d files and paths read from .ninja files. This should
then be controlled by a top-level variable.

This patch changes behavior, but it only has an effect on generated
header files, which aren't common, and it only affects -t msvc, which
is still marked as experimental. (cmake doesn't use it yet.)

(If a file has both `#include <windows.h>` and `<Windows.h>`, this
will now take 2 stat() calls instead of just one, but that should
have a negligible cost.)
@nico
Copy link
Collaborator Author

nico commented May 29, 2013

I verified that this fixes https://crbug.com/242397.

@sgraham

@buildhive
Copy link

Evan Martin » ninja #490 SUCCESS
This pull request looks good
(what's this?)

@evmar
Copy link
Collaborator

evmar commented May 29, 2013

LGTM, but wanted you to double check / fix that one thing so I didn't click merge.

@sgraham
Copy link
Contributor

sgraham commented May 29, 2013

If you really want to avoid ::tolower, the SystemInclude filtering is probably unnecessary (I don't hit that path when using toolchain.py anyway) and we could add an explicit list of extensions in the EndsWith code.

nico added a commit that referenced this pull request May 29, 2013
Stop `-t msvc -o` from lowercasing paths from /showIncludes output.
@nico nico merged commit 4df1510 into ninja-build:master May 29, 2013
@nico nico deleted the wincase branch May 29, 2013 17:20
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.

4 participants