Page MenuHomePhabricator

[PATCH] Do not create a clang-install target for MSVC solutions
ClosedPublic

Authored by aaron.ballman on Nov 9 2015, 10:28 AM.

Details

Reviewers
beanz
Summary

When generating an MSVC solution from CMake, it creates a clang-install project. We do not have any *-install targets as part of MSVC, so this seems out of place (not to mention, a bit strange). This patch disables creation of the clang-install project when making an MSVC solution.

Diff Detail

Event Timeline

aaron.ballman retitled this revision from to [PATCH] Do not create a clang-install target for MSVC solutions.
aaron.ballman updated this object.
aaron.ballman added a reviewer: beanz.
aaron.ballman added a subscriber: cfe-commits.
bcraig added a subscriber: bcraig.Nov 9 2015, 10:49 AM

Does this affect the presence or functionality of the "INSTALL" project?

Does this affect the presence or functionality of the "INSTALL" project?

There is no INSTALL project in MSVC from what I can see (before or after this patch), so I don't think that this affects that functionality.

Does this affect the presence or functionality of the "INSTALL" project?

There is no INSTALL project in MSVC from what I can see (before or after this patch), so I don't think that this affects that functionality.

I spoke too soon, I found it by squinting harder. It's in the CMakePredefinedTargets folder. What do INSTALL and PACKAGE actually *do*? For me, PACKAGE fails to build (CPack error : Cannot find NSIS compiler makensis: likely it is not installed, or not in your PATH), and INSTALL fails with the expected outcome (CMake Error at cmake_install.cmake: 31 (file): file INSTALL cannot make directory "C:/Program Files (x86)/LLVM/include/llvm": No such file or directory

I do not think we should have an INSTALL target for MSVC at all (unless it is building an installer, for instance). Installation requires user input, and likely also requires elevation. Having a project that does the wrong thing (for instance, all of my applications live on D:, not C:) doesn't seem fruitful as a project that is generated by default (but it could be useful to some people to be enabled via a CMake flag).

In a text file, I have a big long cmake invocation. Part of that invocation is the following:

-DCMAKE_INSTALL_PREFIX=c:/install/Tools

The VC "INSTALL" project copies files there, in a very unix-y hierarchy (i.e. c:\install\Tools\bin, c:\install\Tools\include, c:\install\Tools\etc, and so on).

beanz accepted this revision.Nov 9 2015, 2:04 PM
beanz edited edge metadata.
beanz added inline comments.
tools/driver/CMakeLists.txt
58

Can you change this to if(NOT CMAKE_CONFIGURATION_TYPES)?

With that it won't create the install-clang target for any IDE, which is probably the way I should have done this in the first place.

Other than that LGTM.

Thanks,
-Chris

This revision is now accepted and ready to land.Nov 9 2015, 2:04 PM
aaron.ballman added inline comments.Nov 9 2015, 2:07 PM
tools/driver/CMakeLists.txt
58

Thanks, I'll make that modification. Out of curiosity, do you think INSTALL and PACKAGE should be opt-in for MSVC? Not present at all? Always present (as they are currently)?

aaron.ballman closed this revision.Nov 10 2015, 4:54 AM
aaron.ballman marked an inline comment as done.

Committed in r252601.