This is an archive of the discontinued LLVM Phabricator instance.

Explicitly specify CMAKE_AR in WinMsvc.cmake
ClosedPublic

Authored by gmittert on Sep 23 2020, 11:42 AM.

Details

Summary

As of cmake 3.18, cmake changes how it searches for compilers for
Windows (see
https://gitlab.kitware.com/cmake/cmake/-/commit/55196a1440e26917d40e6a7a3eb8d9fb323fa657)
and now finds llvm-ar instead of llvm-lib as CMAKE_AR. This explicitly
specifies CMAKE_AR as llvm-lib so the correct program is found.

Diff Detail

Event Timeline

gmittert created this revision.Sep 23 2020, 11:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2020, 11:42 AM
gmittert requested review of this revision.Sep 23 2020, 11:42 AM
smeenai accepted this revision.Sep 23 2020, 11:55 AM

LGTM, thanks!

It seems strange for CMake to prefer "ar" to "lib" when targeting Windows. Do you happen to know the CMake change that's responsible?

This revision is now accepted and ready to land.Sep 23 2020, 11:55 AM
gmittert updated this revision to Diff 293880.Sep 23 2020, 3:37 PM

I dug a little deeper and found a better way to solve this. Rather than specifying in platforms/WinMsvc, this was specifically while compiling an external project (win runtimes) and CMAKE_AR wasn't being specified properly there.

smeenai accepted this revision.Sep 23 2020, 3:47 PM

LGTM

https://gitlab.kitware.com/cmake/cmake/-/commit/55196a1440e26917d40e6a7a3eb8d9fb323fa657 is the relevant CMake change, and I believe we need to add logic to find llvm-lib. I'll play with that.

Could you update the commit message? (If you updated it locally, you'll need to run arc diff --verbatim to sync it to Phabricator.)

gmittert updated this revision to Diff 293888.Sep 23 2020, 4:07 PM
gmittert edited the summary of this revision. (Show Details)

Updated to include cmake change in commit message.

The LLVMExternalProjectUtils change is needed either way, but I believe the WinMsvc case to be a CMake bug, and I've put up https://gitlab.kitware.com/cmake/cmake/-/merge_requests/5264 to fix it. (We should still go ahead with it as a workaround for now though.)

This revision was automatically updated to reflect the committed changes.
mstorsjo added inline comments.
llvm/cmake/modules/LLVMExternalProjectUtils.cmake
150 ↗(On Diff #293903)

Not familiar with what this cmake file actually does and when it matters, but for mingw setups, the system name would be windows (I presume?) but you'd still want to use llvm-ar.

smeenai added inline comments.Oct 5 2020, 1:20 PM
llvm/cmake/modules/LLVMExternalProjectUtils.cmake
150 ↗(On Diff #293903)

Sorry for the delayed response. This CMake file is for the runtimes build setup (which configures the build to build Clang and then use the just-built Clang to build the runtimes). I think we should just be able to drop this codepath entirely once CMake 3.18.4 comes out with my fix (which teaches CMake to look for llvm-lib when simulating MSVC and stick with llvm-ar otherwise).