libclc: Use temporary files rather than a pipe

[daniels]

Summary

This is required for using the Ninja backend on Windows, as it passes
commands directly to CreateProcess, and does not allow the shell to
interpret them: https://ninja-build.org/manual.html#ref_rule_command

Using the Visual Studio backend is not possible as attempting to create
a static library target comprised entirely of novel languages not known
to the Visual Studio backend built in to CMake's C++ source will
generate nothing at all.

Depends On: D77164

[jvesely]

Windows does have pipes. they work both in cmd and powershell, but perhaps cmake is not using shell to invoke these commands?
What was the generator target you used?

[daniels]

In D77165#1959565, @jvesely wrote:

Windows does have pipes. they work both in cmd and powershell, but perhaps cmake is not using shell to invoke these commands?
What was the generator target you used?

I'm using Ninja, which doesn't support pipes on Windows. The only thing I can think of is that, on Windows, we could execute CMD.EXE /C "clang | llvm-as" in order to keep the pipe, but that would be pretty ugly, and also a bit of a nightmare for quoting.

[daniels]

In D77165#1959786, @daniels wrote:

In D77165#1959565, @jvesely wrote:

What was the generator target you used?

I'm using Ninja [...]

As noted in D17762, we have to use Ninja rather than the VS backend, as the 'standard' way to add new languages for compile in CMake doesn't work with the VS backend. CMake's VS backend has a hardcoded-in-C++-code list of acceptable source languages; when you try to produce a static library with sources in an unknown-to-CMake-VS-backend language, the target ends up completely empty with no actions to produce the output, because the CMake backend doesn't know how.

[jvesely]

In D77165#1959901, @daniels wrote:

In D77165#1959786, @daniels wrote:

In D77165#1959565, @jvesely wrote:

What was the generator target you used?

I'm using Ninja [...]

As noted in D17762, we have to use Ninja rather than the VS backend, as the 'standard' way to add new languages for compile in CMake doesn't work with the VS backend. CMake's VS backend has a hardcoded-in-C++-code list of acceptable source languages; when you try to produce a static library with sources in an unknown-to-CMake-VS-backend language, the target ends up completely empty with no actions to produce the output, because the CMake backend doesn't know how.

It looks like this is a ninja issue. ninja uses sh -c to invoke build rule commands on *nix, but calls CreateProcess directly on windows [0].
[0] suggests prefixing the command with cmd /c, but it doesn't say why that's not the default.
maybe talking to ninja devs might indicate what the preferred way would be.

should we cleanup the .temp file here? does ninja clean leave it behind?
otherwise, I'm OK with this patch with an updated commit message to point to [0] as the real source of the problem.

have you tried using nmake Makefiles?

[0] https://ninja-build.org/manual.html#ref_rule_command

[daniels]

updated commit message

[daniels]

In D77165#1960389, @jvesely wrote:

It looks like this is a ninja issue. ninja uses sh -c to invoke build rule commands on *nix, but calls CreateProcess directly on windows [0].
[0] suggests prefixing the command with cmd /c, but it doesn't say why that's not the default.
maybe talking to ninja devs might indicate what the preferred way would be.

should we cleanup the .temp file here? does ninja clean leave it behind?

I have no idea. I tried to look for a way to declare to CMake that a file was just a temporary artifact and should be deleted, but I couldn't find anything, We could clean it up, but then we'd need to come up with a cross-platform way to remove files (rm vs. rem etc)? If you have any suggestions, I'm all ears.

otherwise, I'm OK with this patch with an updated commit message to point to [0] as the real source of the problem.

Thanks, I've updated the commit message now.

have you tried using nmake Makefiles?

To be honest, I haven't. We already use MSVC builds for most things (preferentially when using CMake), plus Ninja for others, and I'd rather not add NMake as a third build system just to work around this issue.

[jvesely]

I've checked that custom targets are not cleaned up either, so I think it's good to go.
lgtm

[daniels]

Thanks a lot for your reviews @jvesely! Do I need to do anything to get these merged, or are we just waiting on someone else?

libclc/cmake/CMakeLLAsmInformation.cmake

 if(NOT CMAKE_LLAsm_COMPILE_OBJECT)
   set(CMAKE_LLAsm_COMPILE_OBJECT
-    "${CMAKE_LLAsm_PREPROCESSOR} -E -P  <DEFINES> <INCLUDES> <FLAGS> -x cl  <SOURCE> -o - | <CMAKE_LLAsm_COMPILER> -o <OBJECT> ")
+    "${CMAKE_LLAsm_PREPROCESSOR} -E -P  <DEFINES> <INCLUDES> <FLAGS> -x cl <SOURCE> -o <OBJECT>.temp"
+    "<CMAKE_LLAsm_COMPILER> -o <OBJECT> <OBJECT>.temp")
 endif()
 
 if(NOT CMAKE_LLAsm_CREATE_STATIC_LIBRARY)
   set(CMAKE_LLAsm_CREATE_STATIC_LIBRARY
     "<CMAKE_LLAsm_ARCHIVE> -o <TARGET> <OBJECTS>")
 endif()
 
 set(CMAKE_INCLUDE_FLAG_LLAsm "-I")