Improve external include & warning support

[englercj]

What does this PR do?

Adds support for "external" headers to Visual Studio 2022. This adds three new APIs:

• externalwarnings - Controls warning levels for headers considered "external" (/external:Wn, <ExternalWarningLevel>)
• externalanglebrackets - When set to "On" treats all headers included with angle brackets as "external" (/external:anglebrackets, <TreatAngleIncludeAsExternal>)
• externalincludedirs - Include directory paths to treat as "external" (/external:Ipath, <>)

This is a different take on this support than #1701. Here instead of repurposing sysincludedirs, I added a new concept of externalincludedirs. I think these two things are fundamentally different as "external" in this context means "not part of my project" which doesn't necessarily mean "system". For example, I use these for some third-party non-system libraries, which I include using double quotes and not angle brackets.

MSVC Docs: https://docs.microsoft.com/en-us/cpp/build/reference/external-external-headers-diagnostics?view=msvc-170

How does this PR change Premake's behavior?

Existing behavior should be unaffected, all new functionality is behind new APIs.

Anything else we should know?

Add any other context about your changes here.

closes #1692

Did you check all the boxes?

• Focus on a single fix or feature; remove any unrelated formatting or code changes
• Add unit tests showing fix or feature works; all tests pass
• Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
• Follow our coding conventions
• Minimize the number of commits
• Align documentation to your changes

[KyrietS]

Please add your new markdown pages to website/sidebar.js

[nickclark2016]

Out of scope of this PR, but how exactly should this be handled by other build systems? For build systems without this concept, should externalincludedirs be treated as sysincludedirs (I agree with how this is currently being done, just want to make sure we are sure)? I think this question should be answered and the consequences understood before we approve/deny this.

[Jarod42]

I think these two things are fundamentally different as "external" in this context means "not part of my project" which doesn't necessarily mean "system".

Note that msvc does nothing for sysincludedirs, so it might just be a naming issue.
What would you place in "system"?
Aliasing might be an option if really you dislike the name.

[efcy]

I am the author of #1701. Unfortunately I can't continue working on premake for the next few months. However I want to point out that I think it is a bug in premake that sysincluddirs do not supress warnings in visual studio but do in gcc and clang. I wanted to address this as part of my PR.

My idea was to make sysincludedirs behave the same on all systems so I don't need multiple instructions for the same behavior.

But I am okay with closing my PR and going forward with this one. Any support for externalincludedirs is better than none.

[englercj]

What would you place in "system"?

I guess I'd separate include paths I expect to exist on the build system (Windows SDK or glibc++, for example). And external would be dependencies of my application that aren't from the system, for example sqlite, freetype, or other third-party libraries that are external to my app but not part of the system.

Maybe this distinction is moot or undesired, and that's fine I'm open to any solution that lets me tag file paths as external in MSVC so I can disable warnings when including them.

My idea was to make sysincludedirs behave the same on all systems so I don't need multiple instructions for the same behavior.

If I change this PR to fall back to sysincludedirs on other build systems then you will get the behavior you expect of this new instruction silencing warnings on all platforms. Is that a reasonable solution?

[nickclark2016]

I've had some time to dig into the implications of this as opposed to co-opting the existing API. As it currently stands, I don't think I'd want to accept the PR as-is (however I appreciate the work 😃). Below I've listed some of my research into the issue as well as what my take on the correct course of action is.

• externalincludedirs - This is fundamentally no different than sysincludedirs. While this may not semantically be what you want, they have roughly the same meaning. These both seem to mean any include directory other than includes from projects that the user owns.
• externalanglebrackets - This is something unique to Visual Studio as far as I can tell. Since this has no analog to any other toolchain, should we make the API part of the visual studio module or should we ignore it in other toolchains.
• externalwarnings - This equates to the -Wsystem-headers flag in GCC/Clang.

What do I propose?

• I believe that we should coalesce the existing sysincludedirs and externalincludedirs to a single API called externalincludedirs. I think this name works better, as it is the more all-encompassing of the two. This will prevent the need for an exporter-specific API. This would behave by writing externalincludedirs to the externalincludedirs in Visual Studio, but for GNU makefile it would write it to -isystem.
• For externalwarnings, I believe we should try to mirror as closely to the current warnings options as possible. See below table for flag mapping.

Premake Symbol Value	Visual Studio	GNU Makefile
Off	/external:W0
Default	/external:W3	-Wsystem-headers
High	/external:W4	-Wsystem-headers
Extra	/external:W4	-Wsystem-headers
Everything	/external:W4	-Wsystem-headers

• Finally, I believe that the externalanglebrackets API should be specific to Visual Studio.

This would have impacts on the API that already exists for sysincludedirs. Instead of removing the API, I would have it deprecated in favor of the new externalincludedirs and removed in the full release.

Thoughts on this?

[starkos]

+1 on this plan.

This would have impacts on the API that already exists for sysincludedirs. Instead of removing the API, I would have it deprecated in favor of the new externalincludedirs and removed in the full release.

In favor of deprecating, but would probably not remove until v6 at this point, IMHO. Another option is to make sysincludedirs and alias for externalincludedirs?

[englercj]

I believe that we should coalesce the existing sysincludedirs and externalincludedirs to a single API called externalincludedirs

I think this makes a lot of sense, I'll get that done.

For externalwarnings, I believe we should try to mirror as closely to the current warnings options as possible. See below table for flag mapping.

I can make this change, but the reason I had them different originally is because I didn't want the same words with different meanings. For example, "Everything" in the warnings API means /Wall but in the externalwarnings it would mean /W4. That may be surprising for some users so instead of having multiple /W4s I just cut that one out. I'll add it back in for now and let people review.

I also don't like "Default" being /external:W3. The visual studio default is to inherit the project's warning level, which matches the behavior of older versions. I'd like to keep that behavior as the default.

Finally, I believe that the externalanglebrackets API should be specific to Visual Studio.

It is currently only used in vs project generation and ignored by other toolsets. Did you want me to do something different here?

[englercj]

In favor of deprecating, but would probably not remove until v6 at this point, IMHO. Another option is to make sysincludedirs and alias for externalincludedirs?

I don't have strong feelings on this, happy to do whatever people want here. It would probably mean less overall changes to keep sysincludedirs and just change its behavior on VS 2022.

[nickclark2016]

I believe that we should coalesce the existing sysincludedirs and externalincludedirs to a single API called externalincludedirs

I think this makes a lot of sense, I'll get that done.

For externalwarnings, I believe we should try to mirror as closely to the current warnings options as possible. See below table for flag mapping.

I can make this change, but the reason I had them different originally is because I didn't want the same words with different meanings. For example, "Everything" in the warnings API means /Wall but in the externalwarnings it would mean /W4. That may be surprising for some users so instead of having multiple /W4s I just cut that one out. I'll add it back in for now and let people review.

I also don't like "Default" being /external:W3. The visual studio default is to inherit the project's warning level, which matches the behavior of older versions. I'd like to keep that behavior as the default.

For this, I think we should personally make the warnings and externalwarnings flags match as much as possible. To me, this would be the most straight forward. It would be nice to have inherit as well, but I think the mismatch for Default would be confusing.

Finally, I believe that the externalanglebrackets API should be specific to Visual Studio.

It is currently only used in vs project generation and ignored by other toolsets. Did you want me to do something different here?

So that's one option. Another option would be to register the API on the vstudio module. See this file: https://github.com/premake/premake-core/blob/master/modules/vstudio/_preload.lua

[englercj]

OK I think I've addressed all the feedback at this point aside from deprecating sysincludedirs and making it an alias of externalincludedirs. I wanted the naming debate of which one to keep settle first so I knew how to proceed.

[Jarod42]

My opinion about naming:

• externalincludedirs seems clearer than sysincludedirs.
• Having both, one as aliasing of the other is ok.
• sysincludedirs has historic advantage, but deprecating sysincludirs is ok for me.

If we keep only sysincludedirs, then the new externalanglebrackets, externalwarnings should be renamed IMO.

if we deprecated sysincludedirs, how about syslibdirs.

[englercj]

Made changes to deprecate sysincluddirs and only have an implementation for externalincludedirs.

[englercj]

Are there additional changes people are interested in seeing here?

[starkos]

This looks good to me now. I've updated the PR title to be more accurate, and approving for merge.

[starkos]

@englercj Can you please rebase this against the latest main branch, and squash the commits?

[Jarod42]

@starkos:
It seems you miss my comment:

from toolset, it seems we can use "v143" directly instead of "msc-v143", I didn't see code to unify it afterward.
As "vyyy" > "msc-xxx", that check might be problematic. which is the correct method?

[starkos]

@starkos: It seems you miss my comment:

from toolset, it seems we can use "v143" directly instead of "msc-v143", I didn't see code to unify it afterward.
As "vyyy" > "msc-xxx", that check might be problematic. which is the correct method?

I did miss it, thanks for escalating. It looks like the value will get normalized to "msc-v143".

[englercj]

Hey @starkos, just wanted to ping about this comment: #1754 (comment)

I tried the method of comparing cfg.toolset but it is sometimes nil. Please advise.

[starkos]

I'm not sure what to suggest here; I'm not the one who proposed using the toolset value. IMHO I'd go ahead and merge it as is, if you could do us the favor of rebasing your changing against the latest main branch.

[Jarod42]

I suppose p.action.set("vs2010") doesn't set associated toolset.
You probably need to set toolset also afterward :-/

I hope it is only a Unit testing issue.

(I will try to look at it this week-end).

[englercj]

Rebased and ready for merge.

[englercj]

Fixed the toolset check and squashed the commits onto latest master. I think we're good to go.

[peter1745]

Can't wait for this to be merged in 🙂

[englercj]

Rebased atop latest master, ready for merge AFAIK.

[starkos]

Done, thanks!

[efcy]

@englercj was there a reason why its only for visual studio 2022 and up? Externalincludes are supported in vs2019.

[englercj]

@StellaASchlotter It was experimental in most versions of vs2019 and required the /experimental:external flag to be set in order to work. It was only non-experimental as of vs2019 16.10. I didn't deal with the complexity of checking when to set the experimental flag because vs2022 support was sufficient for my needs, and it is guaranteed to work there in all versions.

[efcy]

Ah thanks for this information. I was not aware of this. I will upgrade the VS version in my team then.