-
-
Notifications
You must be signed in to change notification settings - Fork 618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add conditional behavior to global variables, add systemversion as first implementation #1048
Conversation
…The UI can't do this but setting it actually works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not opposed to this change, however, I have thought about this a bit. This kind of falls into the realm of "unsupported" behaviour for me, maybe it works today, but will it continue to work forever? Should Premake take the risk that it will one day just not work?
Given the flexibility of nearly every other IDE/toolchain, I can't imagine Microsoft breaking this, so perhaps I'm just over thinking this. I'll let the others decide if this is something that Premake should do or not.
Thanks for making the change! I think the only thing I would suggest is allowing the version to be set both in the "global" and "configuration" PropertyGroup
blocks. If you set it at the project level, having it in each conditional property group would be overkill - or if you're only changing it for a specific configuration.
Sure, I'm all for having a discussion about it. To me it doesn't sound so much "unsupported" as "not exposed" in the sense that there seem to be many things the .vcxproj can do that aren't in the UI. I totally see your point that there's a kind of risk in exposing these kinds of things, but my feeling is it looks more like a limitation of the UI than a limitation of the xml system they have in place. Perhaps it would be worthwhile to see what cmake does or try to find some better documentation coming from MS. Regarding the changes, I'll make it more flexible so it can go in Globals or Config depending on settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm with @redorav on this one. The UI doesn't always keep up with the capabilities of MSBuild.
I do think we need to keep setting the Global block though. For most users, the one setting is sufficient, and having it in Global allows them to continue changing it through the UI. Can you conditionally emit the configuration blocks only if they differ from the global setting?
Thanks for the feedback @starkos and @samsinsane , I'm currently investigating how to expand the Globals section to include conditionals as needed. The interesting thing about these modifications is that they are actually tweakable via the UI provided the platform/configuration condition was present there to begin with, so it's not like we're taking anything away from the user. |
…entries based on configuration
I've moved systemversion back to Globals, and also create conditional global entries in the vcxproj. One thing I've not found a straightforward way of doing is to not create the property groups if no entries are going to be populated, i.e. the general case ends up as: <PropertyGroup Label="Globals">
<ProjectGuid>{42B5DBC6-AE1F-903D-F75D-41E363076E92}</ProjectGuid>
<IgnoreWarnCompileDuplicatedFilename>true</IgnoreWarnCompileDuplicatedFilename>
<Keyword>Win32Proj</Keyword>
<RootNamespace>MyProject</RootNamespace>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'" Label="Globals">
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'" Label="Globals">
</PropertyGroup> I think the assumption is that there's always something that's going to populate it. Any suggestions? I've added an example test, but I mean to expand it once we agree on a final implementation. |
If you search the source for "io.capture" you can find some places where we do that. You run the code and capture the output, and then check to see if the output is empty before writing it out. Let me know you need help with it and I'll throw together a proper example. |
…sult of the global configuration property function. If there's nothing to capture, don't emit.
… into systemversion
I think I managed to do it via premake.capture() which I guess was renamed at some point from io.capture. This opens up the avenue for the Android bug #1038 so we can set it conditionally. |
As soon as you're happy with the changes I've also fixed #1038 locally which relies on this PR to work, since this one allows setting conditions on globals and the other fix extends the function via premake.override. |
@samsinsane, are you okay with this one? If so, feel free to merge. |
The UI can't do this but setting it manually actually works. Fixes #936