Support paths with spaces when building ninja extension

[ppwwyyxx]

Generate the following build.ninja file and can successfully build:

cflags = -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -fPIC -DWITH_CUDA '-I/scratch/yuxinwu/space space/detectron2/layers/csrc' -I/private/home/yuxinwu/miniconda3/lib/python3.7
/site-packages/torch/include -I/private/home/yuxinwu/miniconda3/lib/python3.7/site-packages/torch/include/torch/csrc/api/include -I/private/home/yuxinwu/miniconda3/lib/python3.7/site-packages/torc
h/include/TH -I/private/home/yuxinwu/miniconda3/lib/python3.7/site-packages/torch/include/THC -I/public/apps/cuda/10.1/include -I/private/home/yuxinwu/miniconda3/include/python3.7m -c
post_cflags = -DTORCH_API_INCLUDE_EXTENSION_H -DTORCH_EXTENSION_NAME=_C -D_GLIBCXX_USE_CXX11_ABI=0 -std=c++14
cuda_cflags = -DWITH_CUDA '-I/scratch/yuxinwu/space space/detectron2/layers/csrc' -I/private/home/yuxinwu/miniconda3/lib/python3.7/site-packages/torch/include -I/private/home/yuxinwu/miniconda3/li
b/python3.7/site-packages/torch/include/torch/csrc/api/include -I/private/home/yuxinwu/miniconda3/lib/python3.7/site-packages/torch/include/TH -I/private/home/yuxinwu/miniconda3/lib/python3.7/site
-packages/torch/include/THC -I/public/apps/cuda/10.1/include -I/private/home/yuxinwu/miniconda3/include/python3.7m -c
cuda_post_cflags = -D__CUDA_NO_HALF_OPERATORS__ -D__CUDA_NO_HALF_CONVERSIONS__ -D__CUDA_NO_HALF2_OPERATORS__ --expt-relaxed-constexpr --compiler-options '-fPIC' -DCUDA_HAS_FP16=1 -D__CUDA_NO_HALF_
OPERATORS__ -D__CUDA_NO_HALF_CONVERSIONS__ -D__CUDA_NO_HALF2_OPERATORS__ -ccbin=/public/apps/gcc/7.1.0/bin/gcc -DTORCH_API_INCLUDE_EXTENSION_H -DTORCH_EXTENSION_NAME=_C -D_GLIBCXX_USE_CXX11_ABI=0
-gencode=arch=compute_60,code=sm_60 -gencode=arch=compute_70,code=sm_70 -std=c++14
ldflags =

rule compile
  command = $cxx -MMD -MF $out.d $cflags -c $in -o $out $post_cflags
  depfile = $out.d
  deps = gcc

rule cuda_compile
  command = $nvcc $cuda_cflags -c $in -o $out $cuda_post_cflags



build /scratch/yuxinwu/space$ space/build/temp.linux-x86_64-3.7/scratch/yuxinwu/space$ space/detectron2/layers/csrc/vision.o: compile /scratch/yuxinwu/space$ space/detectron2/layers/csrc/vision.c$
p
build /scratch/yuxinwu/space$ space/build/temp.linux-x86_64-3.7/scratch/yuxinwu/space$ space/detectron2/layers/csrc/box_iou_rotated/box_iou_rotated_cpu.o: compile /scratch/yuxinwu/space$ space/de$
ectron2/layers/csrc/box_iou_rotated/box_iou_rotated_cpu.cpp
build /scratch/yuxinwu/space$ space/build/temp.linux-x86_64-3.7/scratch/yuxinwu/space$ space/detectron2/layers/csrc/ROIAlignRotated/ROIAlignRotated_cpu.o: compile /scratch/yuxinwu/space$ space/de$
ectron2/layers/csrc/ROIAlignRotated/ROIAlignRotated_cpu.cpp
build /scratch/yuxinwu/space$ space/build/temp.linux-x86_64-3.7/scratch/yuxinwu/space$ space/detectron2/layers/csrc/nms_rotated/nms_rotated_cpu.o: compile /scratch/yuxinwu/space$ space/detectron2$
layers/csrc/nms_rotated/nms_rotated_cpu.cpp
build /scratch/yuxinwu/space$ space/build/temp.linux-x86_64-3.7/scratch/yuxinwu/space$ space/detectron2/layers/csrc/ROIAlign/ROIAlign_cpu.o: compile /scratch/yuxinwu/space$ space/detectron2/layer$
/csrc/ROIAlign/ROIAlign_cpu.cpp

[dr-ci]

💊 CI failures summary and remediations

As of commit eb92828 (more details on the Dr. CI page):

---

💚 💚 Looks good so far! There are no failures yet. 💚 💚

---

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 40 times.

[zou3519]

@ppwwyyxx I can take a more detailed look tomorrow (I'm thinking about if we need to quote any other flags, and if we can add a test for this change), but do you know if previous versions of PyTorch supported building with paths with spaces?

[ppwwyyxx]

The build did not work before if the paths of the extension contains spaces

[zou3519]

@ppwwyyxx as a quick workaround, I think the "no ninja" build is able to handle paths with spaces. You can set the option in a setup.py file like in the following:

pytorch/test/cpp_extensions/setup.py

Line 62 in 11a4041

cmdclass={'build_ext': BuildExtension.with_options(use_ninja=USE_NINJA)})

[zou3519]

Thanks for the PR, @ppwwyyxx. I added some suggestions for where to put the "shlex.quote" code.

It would be nice to add a test to prevent regressions as well, please see my comments there.

[ppwwyyxx]

I found that the sccache wrapper in the CI docker image does not support paths with spaces, causing all CI to fail.

Specifically, the content of /opt/cache/bin/cc is:

#!/bin/sh
exec sccache /usr/bin/cc $*

now, but it should be

#!/bin/sh
exec sccache /usr/bin/cc "$@"

to handle argument with spaces. Same for all wrappers under /opt/cache/bin/.

However I don't know how I can update the sccache wrapper. It seems beyond what I can do in this PR..

[zou3519]

@ppwwyyxx thanks for digging into it!

Given that it's really hard to write a test for this, I am inclined to say we should forget about the tests (and revert our changes to those), and then ship the changes as-is because they are an improvement. Since this would be untested, we won't advertise that this functionality is possible and there is the risk that it will regress. I'll put up an issue after this one gets merged to figure out how to actually add a test.

Does that sound reasonable?

[ppwwyyxx]

Sounds good! I've removed the test

[zou3519]

lgtm, thank you!

[zou3519]

@ppwwyyxx there's a merge conflict, can you rebase this PR? After that I can merge it (you can also merge it if you'd like, I wasn't sure if you had experience with that)

[facebook-github-bot]

@ppwwyyxx is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@ppwwyyxx merged this pull request in 0e2a047.