Support LLVM platform toolset for MSC; Clang in Visual Studio. #169

TurkeyMan · 2015-07-25T08:53:55Z

Installing the LLVM bundle these days integrates Clang with VS.

The method is different than the popular vs-tool plugin which populates the project with Clang compiler settings, instead LLVM installs a shim for MSC.
I feel the best way to express this in premake is to retain toolset='msc', and support it like the other VS platform toolset options.

I have added support the same way we can use toolset="v100"; use toolset="LLVM_vs2010" instead.

toolset="msc-LLVM-vs2010" is also supported. I'm not sure this is the best expression, but I can't think of a more appropriate one.

TurkeyMan · 2015-07-26T01:14:57Z

src/actions/vstudio/vs2010_vcxproj.lua

@@ -1501,7 +1503,9 @@
 	function m.platformToolset(cfg)
 		local tool, version = p.config.toolset(cfg)
 		if version then
-			version = "v" .. version
+			if tonumber(version) ~= nil then
+				version = "v" .. version


@starkos Are you particularly attached to the functionality of canonical() where it returns 120 instead of v120?
Why not just make that function return v120, rather than stripping the 'v', and then adding it back on here?

TurkeyMan · 2015-07-26T14:12:16Z

src/actions/vstudio/vs2010_vcxproj.lua

@@ -1501,7 +1503,9 @@
 	function m.platformToolset(cfg)
 		local tool, version = p.config.toolset(cfg)
 		if version then
-			version = "v" .. version
+			if tonumber(version:sub(1,3)) ~= nil then
+				version = "v" .. version


@starkos Are you particularly attached to the functionality of canonical() where it returns 120 instead of v120?
Why not just make that function return v120, rather than stripping the 'v', and then adding it back on here?

No, I'm not. Just ended up that way because of the order in which the feature got implemented.

So, I notice you didn't merge this, would you like me to change that?

starkos · 2015-07-27T21:54:40Z

Sorry, I ran out of time this weekend. I'd like to give this a test run against some of my client code to make sure there are no side effects before I merge it, as we use this feature a lot.

What about just toolset("clang") though? Since we know we're generating for Visual Studio, can we just infer the rest?

TurkeyMan · 2015-07-27T22:33:26Z

Like I said in the opener, I think toolset "clang" means something different.
LLVM installs a proxy cl.exe, and adds an LLVM platform toolset, such that even though we're building with clang, we're still really compiling with MSC; ie, command line is regular MSC command line, and Clang is specifically configured to imitate MSC as best it can, and we're limited to options the MSC command line is able to express.
For this reason, I don't think it's right to state toolset "clang", since it's kinda not. It really just looks like a platform toolset with the MSC compiler.

Alternatively, I have a vs-tool extension, which is a popular plugin (Mozilla and Google endorse it). It supports Clang and GCC in VS properly; it replaces the C++ and Linker settings in the project files with different Clang/GCC settings, and populates the project options with all the true options relating to those compilers. When you build you're actually invoking GCC/Clang directly, and that's the context I think best suits toolset "clang"/"gcc" with the vs* actions.

For my money, vs-tool gives a much better result, but it's a more comprehensive addon. I don't feel too concerned about adding support for this platform toolset version in core, since it's just a couple of lines.
Both solutions have their place.

You were in favour of integration the vs-tool stuff at one point a while back. Would you still consider that?

starkos · 2015-07-28T23:34:17Z

Okay, I get it.

What would make the most sense to me would be to require the leading msc-:

toolset "msc-LLVM-vs2010"
toolset "msc-LLVM-vs2014_xp"

Then modify p.tools.canonical() to only consider the first dash when splitting, leaving everything after intact. So the original code would give you parts = { "msc", "LLVM" }; only considering the first dash gets you parts = { "msc", "LLVM-vs2010" } and everything is easier. And this is consistent with setting the MS platform toolset versions:

toolset "msc-110"
toolset "msc-120"

TurkeyMan · 2015-07-28T23:59:36Z

Why require the leading msc? I'm not sure what value that adds... People will be surprised when typing a valid platform toolset alone works v120, but then typing another valid toolset alone LLVM-vs2010 doesn't work. I'm definitely for uniformity of behaviour in this case.

starkos · 2015-07-29T22:47:38Z

I'm definitely for uniformity of behaviour in this case.

Actually the "v" form is the weird one. I know it isn't implemented yet, but for other compilers it would be "clang-(version)" and "gcc-(version). "msc-llvm-vs2010" is following the common behavior and the API documentation.

I added the "v" option because I know that a few large Premake installs had already adopted that convention and I wanted them to be able to use the public version without modification. Perhaps I should de-emphasize that in the documentation?

TurkeyMan · 2015-07-30T12:44:18Z

Okay, I see.
Well either way I'm happy, I just don't like the non-uniformity :)
I guess moving away from the lone v120 anomaly is the more principled option in this case. If you want to go that way?

starkos · 2015-07-30T22:40:58Z

If you're okay with it, I say go the msc- route and I will de-emphasize the v option in the documentation. I'd like to leave support for the v in code though.

starkos · 2015-08-23T19:37:34Z

Ping?

TurkeyMan · 2015-08-24T12:32:16Z

Hey, yeah, sorry for my radio silence lately! I've started a new and super aggressive project at work with tonnes of overtime. Coupled with endless social commitments, I've barely touched a computer in my spare time! >_<

The reason I haven't fixed this up, is that my plan was to add an optional parameter to explode which will limit explosion to a maximum number of tokens.

function string.explode(s, pattern, plain, maxTokens)

Something like that maybe? I wasn't sure if the order of the last 2 parameters would be better swapped around.

I've wanted this function in a couple of places, it seemed like a nice tweak.
Seem reasonable?

starkos · 2015-08-24T16:01:49Z

Sounds good to me, and I think that is the correct order for the arguments.

TurkeyMan · 2015-09-01T12:48:23Z

Finally got to it!

TurkeyMan · 2015-09-01T23:07:44Z

Fixes #238

kjk · 2015-09-01T23:25:24Z

Looks good (I like the additional touch of auto-removal of cl.exe options not supported by clang) although from discussion I can't tell if "LLVM-vs2014_xp" will be supported or not.

It would be weird and I'm sure a constant generator of confusion for people if "LLVM-vs2014_xp" wasn't recognized since this is the toolset name that shows up in Visual Studio UI drop-down. It's fine if there's "msc-*" alias but the name that shows up in VS UI should also be valid.

TurkeyMan · 2015-09-01T23:26:09Z

It's supported.

TurkeyMan · 2015-09-01T23:29:18Z

Sorry, misread; @starkos wanted to move away from toolset specifications on their own, ie, without msc- preceding. I was going to remove v100 support, but starkos said he was using it.
Toolset should be in the format: toolset-version, where for MSC, version is the platform toolset.

kjk · 2015-09-01T23:30:46Z

BTW, when I tried to compile SumatraPDF with clang's 3.7 cl.exe, those were the options that clang.exe didn't recognize:

2>clang-cl.exe : error : argument unused during compilation: '/MP'
2>clang-cl.exe : error : argument unused during compilation: '/GL'
2>clang-cl.exe : error : argument unused during compilation: '/Gm-'
2>clang-cl.exe : error : argument unused during compilation: '/GS'

They came from: https://github.com/sumatrapdfreader/sumatrapdf/blob/master/premake5.lua (and I manually edited .csproj files to replace toolset).

From reading the code it looks like "/GS" will be auto-removed but not the other 3. Not a big deal, since I can work-around this in premake file.

starkos · 2015-09-04T15:42:13Z

@kjk : Take a look at my explanation for the msc- requirement above. Does it make more sense in the context of gcc and clang?

kjk · 2015-09-04T18:40:24Z

@starkos It might be a perfectly logical scheme. I just believe people will be confused if they see "LLVM-vs2014" toolset available in Visual Studio UI but the same value in "toolset" command will cause an error.

You might be assuming that they will either read (and memorize) all premake docs before writing premake files or in case of error will go to (yet to be written) explanation of why "toolset" doesn't accept what they expect to be valid but what is also likely to happen is that they open a bug here or ask a question on stackoverflow.

For example, when I encountered this, I assumed it's a bug in premake and opened a bug. It didn't even occur to me that this could be supported but under a different name so I didn't bother (re)reading docs on "toolset". Even if "msc-*" was available, I would probably end up opening a bug.

TurkeyMan · 2015-09-05T07:25:21Z

@kjk Okay, I didn't have them appear in my projects. Can I get to them in a separate PR? I'd like to see this merged before messing with stuff like that.

starkos · 2015-09-09T19:06:50Z

Thanks for this!

Support LLVM platform toolset for MSC; Clang in Visual Studio.

bindirs are added to make files for gmake2

TurkeyMan force-pushed the llvm_toolset branch from 057012e to dda6cb5 Compare July 25, 2015 08:56

TurkeyMan reviewed Jul 26, 2015
View reviewed changes

TurkeyMan force-pushed the llvm_toolset branch from febac04 to dd66655 Compare July 26, 2015 14:11

TurkeyMan reviewed Jul 26, 2015
View reviewed changes

TurkeyMan added 2 commits August 2, 2015 12:34

Support LLVM platform toolset for MSC; Clang in Visual Studio.

eab0933

Silence warnings about arguments unknown to Clang, fixed a mistake

04a7cd1

TurkeyMan force-pushed the llvm_toolset branch from dd66655 to 931ff46 Compare September 1, 2015 12:37

string.explode() gains 'maxTokens' argument.

28ebe8f

TurkeyMan force-pushed the llvm_toolset branch from 931ff46 to 28ebe8f Compare September 1, 2015 12:46

TurkeyMan mentioned this pull request Sep 1, 2015

clang toolset for Visual Studio not supported #238

Closed

starkos added a commit that referenced this pull request Sep 9, 2015

Merge pull request #169 from TurkeyMan/llvm_toolset

b5ee3ac

Support LLVM platform toolset for MSC; Clang in Visual Studio.

starkos merged commit b5ee3ac into premake:master Sep 9, 2015

TurkeyMan deleted the llvm_toolset branch September 12, 2015 04:32

TurkeyMan pushed a commit that referenced this pull request Feb 16, 2018

Fix gmake2 (#169)

0f5cf98

bindirs are added to make files for gmake2

TurkeyMan pushed a commit to Blizzard/premake-core that referenced this pull request Jun 2, 2018

Fix gmake2 (premake#169)

be75812

bindirs are added to make files for gmake2

TurkeyMan pushed a commit to Blizzard/premake-core that referenced this pull request Jun 5, 2018

Fix gmake2 (premake#169)

9219bd6

bindirs are added to make files for gmake2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support LLVM platform toolset for MSC; Clang in Visual Studio. #169

Support LLVM platform toolset for MSC; Clang in Visual Studio. #169

TurkeyMan commented Jul 25, 2015

TurkeyMan Jul 26, 2015

TurkeyMan Jul 26, 2015

starkos Jul 26, 2015

TurkeyMan Jul 26, 2015

starkos commented Jul 27, 2015

TurkeyMan commented Jul 27, 2015

starkos commented Jul 28, 2015

TurkeyMan commented Jul 28, 2015

starkos commented Jul 29, 2015

TurkeyMan commented Jul 30, 2015

starkos commented Jul 30, 2015

starkos commented Aug 23, 2015

TurkeyMan commented Aug 24, 2015

starkos commented Aug 24, 2015

TurkeyMan commented Sep 1, 2015

TurkeyMan commented Sep 1, 2015

kjk commented Sep 1, 2015

TurkeyMan commented Sep 1, 2015

TurkeyMan commented Sep 1, 2015

kjk commented Sep 1, 2015

starkos commented Sep 4, 2015

kjk commented Sep 4, 2015

TurkeyMan commented Sep 5, 2015

starkos commented Sep 9, 2015

Support LLVM platform toolset for MSC; Clang in Visual Studio. #169

Support LLVM platform toolset for MSC; Clang in Visual Studio. #169

Conversation

TurkeyMan commented Jul 25, 2015

TurkeyMan Jul 26, 2015

Choose a reason for hiding this comment

TurkeyMan Jul 26, 2015

Choose a reason for hiding this comment

starkos Jul 26, 2015

Choose a reason for hiding this comment

TurkeyMan Jul 26, 2015

Choose a reason for hiding this comment

starkos commented Jul 27, 2015

TurkeyMan commented Jul 27, 2015

starkos commented Jul 28, 2015

TurkeyMan commented Jul 28, 2015

starkos commented Jul 29, 2015

TurkeyMan commented Jul 30, 2015

starkos commented Jul 30, 2015

starkos commented Aug 23, 2015

TurkeyMan commented Aug 24, 2015

starkos commented Aug 24, 2015

TurkeyMan commented Sep 1, 2015

TurkeyMan commented Sep 1, 2015

kjk commented Sep 1, 2015

TurkeyMan commented Sep 1, 2015

TurkeyMan commented Sep 1, 2015

kjk commented Sep 1, 2015

starkos commented Sep 4, 2015

kjk commented Sep 4, 2015

TurkeyMan commented Sep 5, 2015

starkos commented Sep 9, 2015