Page MenuHomeWildfire Games

Remove hardcoded minimum OSX version in premake5
ClosedPublic

Authored by Itms on Nov 9 2018, 7:27 PM.

Details

Summary
Test Plan

Tested by @trompetin17 (running and bundling)

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

trompetin17 created this revision.Nov 9 2018, 7:27 PM
Owners added a subscriber: Restricted Owners Package.Nov 9 2018, 7:27 PM
Vulcan added a subscriber: Vulcan.Nov 9 2018, 7:35 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/772/

Can the -mmacosx-version-min=10.9 flag be replaced by a constant? To prevent those big changes to change a version.

Itms requested changes to this revision.Nov 9 2018, 8:48 PM
Itms added a subscriber: Itms.

Yes, like Vlad says, we should change our own defaults in the premake scripts, but we mustn't touch premake source (it's easier to maintain when we match upstream).

This revision now requires changes to proceed.Nov 9 2018, 8:48 PM
In D1669#66044, @Itms wrote:

Yes, like Vlad says, we should change our own defaults in the premake scripts, but we mustn't touch premake source (it's easier to maintain when we match upstream).

The source code actually has -mmacosx-version-min=10.4, so like i understand is that change to something like -mmacosx-version-min=${MIN_OSX_VERSION}?

trompetin17 updated this revision to Diff 6980.Nov 10 2018, 4:01 AM

Added environment variable instead hardcode version.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/778/

trompetin17 updated this revision to Diff 6988.Tue, Nov 13, 7:59 PM

Remove -mmacosx-version-min like itms suggest, and update to use osx 10.9

Itms requested changes to this revision.Tue, Nov 13, 9:44 PM

Regarding the premake5 change, it looks good, you could remove the offending line from build/premake/premake5/premake5.lua for completeness. I hope to get a quick answer on issue 1154.

build/workspaces/update-workspaces.sh
71 ↗(On Diff #6988)

No, this won't work, our premake script doesn't use this environment variable.

125 ↗(On Diff #6988)

It's here that --macosx-version-min="10.9" or --macosx-version-min="${MIN_OSX_VERSION}" should be passed if we are on OSX.

This revision now requires changes to proceed.Tue, Nov 13, 9:44 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/788/

Itms commandeered this revision.Sun, Dec 2, 2:40 PM
Itms edited reviewers, added: trompetin17; removed: Itms.
Itms updated this revision to Diff 7015.Sun, Dec 2, 2:42 PM
Itms retitled this revision from [OSX] Package and build Mojave to Remove hardcoded minimum OSX version in premake5.
Itms edited the summary of this revision. (Show Details)
Itms edited the test plan for this revision. (Show Details)

Final version.

Vulcan added a comment.Sun, Dec 2, 2:44 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/808/

Stan added a subscriber: Stan.Sun, Dec 2, 7:41 PM

Might I suggest to save this as a patch and apply it on premake when updating update workspaces on OsX ?

Itms added a comment.Sun, Dec 2, 8:04 PM
In D1669#66846, @Stan wrote:

Might I suggest to save this as a patch and apply it on premake when updating update workspaces on OsX ?

We would do that if we were pulling premake from somewhere, but here we keep a copy of premake in the tree, so we should just patch that copy.

Stan added a comment.Sun, Dec 2, 8:22 PM

I see. Won't be hard to keep track of all the patches we apply ? For now we only have one but we might have more in the future, no ?

Itms added a comment.Sun, Dec 2, 8:40 PM
In D1669#66853, @Stan wrote:

I see. Won't be hard to keep track of all the patches we apply ? For now we only have one but we might have more in the future, no ?

Yes, please read the discussion at https://github.com/premake/premake-core/issues/1154. The correct fix would be to upgrade to alpha13 of premake5, but the odds of breaking the build system are a bit high, so I'll upgrade after the release.

Stan added a comment.Sun, Dec 2, 8:45 PM

Sounds fair then. Thanks for taking time to elaborate :)

This revision was not accepted when it landed; it landed in state Needs Review.Sun, Dec 2, 9:58 PM
This revision was automatically updated to reflect the committed changes.