Page MenuHomePhabricator

Use relative path to clang-cl
ClosedPublic

Authored by zturner on Feb 28 2017, 11:29 AM.

Details

Summary

This is an attempt to get the fix the issue breaking the sanitizer windows bots (http://lab.llvm.org:8011/builders/sanitizer-windows/). I put a comment in explaining why I think this might work.

I'm not sure how else to fix it. If we want to use backslashes it has to be 4 backslashes. We could also use forward slashes. But we don't have the value of %(workdir) on the master, so we can't know. In one thread Galina suggested to use file(TO_CMAKE_PATH) but this would need to be done at the top level LLVM CMakeLists.txt file, which is a little scary, and furthermore CMake officially marks this method as "avoid" (reference: https://cmake.org/Wiki/CMake_FAQ#How_do_I_use_a_different_compiler.3F)

That said, if

file(TO_CMAKE_PATH "${CMAKE_CXX_COMPILER}" CMAKE_CXX_COMPILER)
file(TO_CMAKE_PATH "${CMAKE_C_COMPILER}" CMAKE_C_COMPILER)

works and will not cause unforeseen problems, I'm open to trying it. But this is beyond my CMake expertise. +beanz and brad king who might be able to comment on the CMake method.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.Feb 28 2017, 11:29 AM
brad.king edited edge metadata.Feb 28 2017, 11:46 AM

If the CMAKE_C_COMPILER value is not provided to CMake then its logic for initializing it is part of the enable-language logic done inside the project or enable_language command. Therefore in general project code should not try to touch it.

CMAKE_C_COMPILER is also not something that all generators support having set in the cache. The VS IDE and Xcode generators just provide the value for reference but do not use it during generation.

If the CMAKE_C_COMPILER value is provided to CMake and one is using a generator that allows this (Makefile, Ninja) then it is expected to use forward slashes currently. However, our _cmake_find_compiler_path macro is always called when CMAKE_C_COMPILER is externally provided in order to make sure we can find its absolute path. Perhaps that could be taught to use file(TO_CMAKE_PATH) too. Patch welcome!

zturner updated this revision to Diff 90074.Feb 28 2017, 12:50 PM

Looks like file(TO_CMAKE_PATH) won't be an ideal solution then.

I inspected the buildbot code a little more closely and we pass workdir=build_fuzzer_dir, whereas the clang is in build_dir. So I prepend a .. so that the relative path is correct.

rnk edited edge metadata.Mar 1 2017, 8:52 AM

I connected to the bot and this doesn't work. I got this output: https://ghostbin.com/paste/uyj36

The CMAKE_C_COMPILER: ../build/bin/clang-cl.exe is not a full path and was not found in the PATH

CMake expects that if this is set then it is an absolute path with forward slashes. Either the infrastructure here will need to learn how to give %(workdir) forward slashes, or the change to CMake I mentioned previously will need to be made.

Alternatively, put the path to clang-cl in the PATH and set CMAKE_C_COMPILER=clang-cl.

zturner updated this revision to Diff 90192.Mar 1 2017, 9:02 AM

Previous method didn't work, but in the step right before the failing step we are already adding the bin directory to the system's PATH. So just specify clang-cl.exe with no path at all, and CMake should be able to find it.

rnk added a comment.Mar 1 2017, 9:04 AM

Alternatively, put the path to clang-cl in the PATH and set CMAKE_C_COMPILER=clang-cl.

I just got back into the office and Zach and I both came to this conclusion as well. This buildbot code is already putting the appropriate bin directory on PATH anyway. Thanks for the help!

rnk accepted this revision.Mar 1 2017, 9:05 AM

Looks good, ship it. :)

I'm going to try to manually run through the steps on the bot and I'll let @gkistanova know when we're ready for a restart.

This revision is now accepted and ready to land.Mar 1 2017, 9:05 AM
This revision was automatically updated to reflect the committed changes.