Page MenuHomePhabricator

libclc: Use temporary files rather than a pipe
ClosedPublic

Authored by daniels on Mar 31 2020, 12:30 PM.

Details

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

Diff Detail

Event Timeline

daniels created this revision.Mar 31 2020, 12:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2020, 12:30 PM

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?

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.

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.

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 this revision to Diff 255491.Apr 6 2020, 2:20 PM

updated commit message

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.

daniels edited the summary of this revision. (Show Details)Apr 6 2020, 2:26 PM
jvesely accepted this revision.Apr 10 2020, 12:16 AM

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

This revision is now accepted and ready to land.Apr 10 2020, 12:16 AM

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?

This revision was automatically updated to reflect the committed changes.