Escape spaces in the depfile output #2503
Conversation
|
Filed issue #2504 which this PR can resolve. |
|
r? @fitzgen |
|
@danakj excuse me if I'm missing something but is it not more straightforward to use |
|
Yes, it would. Cool, I failed to find that function. Though I will note that it will require some String allocations to use that instead of simply iterating over the chars. Probably.. this is fine..? If you're able to review/approve, then I assume you have a preference for it and I can switch to that. Otherwise, I can do either one. |
|
Yeah you'd have to do several allocations to handle all the escapes which is not ideal but I don't expect this part of the code to be performance critical and I'd be more inclined to have shorter and more concise code handling this. I can do a proper review later, let me know when the |
|
Ok thanks for the quick feedback. I've used |
pvdrz
left a comment
pvdrz
left a comment
There was a problem hiding this comment.
There are some clippy warnings about using single character strings for patterns. e.g. using " " instead of ' '. Could you fix those?
Also could you add a new entry to CHANGELOG.md explaining this change?
danakj
force-pushed
the
escape-spaces
branch
2 times, most recently
from
3439b8f to
6971cb0
Compare
Header file paths included from C/C++ files may include spaces in them, and if they do, the spaces need to be escaped in order to generate a proper depfile. Otherwise, the dep file ends up pointing to non-existant paths. For example: ../../third_party/depot_tools/win_toolchain/vs_files/27370823e7\Windows Kits\10\Include\10.0.22621.0\shared\winapifamily.h This path ends up being parsed as two files, neither of which exist: ../../third_party/depot_tools/win_toolchain/vs_files/27370823e7\Windows Kits\10\Include\10.0.22621.0\shared\winapifamily.h This leads to build failures with ninja when it's verifying that the build is complete: ``` $ ninja -C out rust_mojo_unittests -w dupbuild=err -n -d explain ninja explain: output ../../third_party/depot_tools/win_toolchain/vs_files/27370823e7\Windows doesn't exist ninja explain: output Kits/10/Include/10.0.22621.0/shared/winapifamily.h is dirty ``` ninja accepts spaces being escaped with backslash, so the path Windows Kits\10\Include\10.0.22621.0\shared\winapifamily.h can be valid if escaped as: Windows\ Kits\10\Include\10.0.22621.0\shared\winapifamily.h However this is less clear for humans as there is inconsistent use of backslashes, so we opt to escape backslashes as well, which ninja also accepts, and is standard for Windows paths. Windows\ Kits\\10\\Include\\10.0.22621.0\\shared\\winapifamily.h The same is also true for the target name, or `output_module` as its named in bindgen. Issue ninja-build/ninja#168 demonstrates this case as well. So we also escape the target name if there is a space present.
|
Clippy should be happy now, and CHANGELOG updated (which meant rebasing). |
|
Thank you for reviewing :) |
2 participants
Header file paths included from C/C++ files may include spaces in them, and if they do, the spaces need to be escaped in order to generate a proper depfile. Otherwise, the dep file ends up pointing to non-existant paths.
For example:
../../third_party/depot_tools/win_toolchain/vs_files/27370823e7\Windows Kits\10\Include\10.0.22621.0\shared\winapifamily.h
This path ends up being parsed as two files, neither of which exist: ../../third_party/depot_tools/win_toolchain/vs_files/27370823e7\Windows Kits\10\Include\10.0.22621.0\shared\winapifamily.h
This leads to build failures with ninja when it's verifying that the build is complete:
ninja accepts spaces being escaped with backslash, so the path Windows Kits\10\Include\10.0.22621.0\shared\winapifamily.h can be valid if escaped as:
Windows\ Kits\10\Include\10.0.22621.0\shared\winapifamily.h
However this is less clear for humans as there is inconsistent use of backslashes, so we opt to escape backslashes as well, which ninja also accepts, and is standard for Windows paths.
Windows\ Kits\10\Include\10.0.22621.0\shared\winapifamily.h