Page MenuHomeWildfire Games

Target build version explicitly for Xcode
ClosedPublic

Authored by wraitii on May 5 2018, 7:00 PM.

Details

Summary

This makes Xcode project target 10.8 by default, instead of the latest OSX (which may actually be more recent than what is running on the host).

(adding you Vladislav, I believe you have OS X)

Test Plan

Somebody on OSX should review this.

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

wraitii created this revision.May 5 2018, 7:00 PM
Owners added a subscriber: Restricted Owners Package.May 5 2018, 7:00 PM
Vulcan added a subscriber: Vulcan.May 5 2018, 7:05 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/461/display/redirect

You need to update premake4.lua too, no?

Also we shouldn't forget to update our wiki too.

Itms requested changes to this revision.Dec 2 2018, 3:04 PM

This should use premake5.lua's macosx-version-min variable in order to avoid hardcoding. With D1685, this will actually be 10.9.

premake4 can be ignored, because macOS compilation is only well tested with premake5 as of A23, and D1275 is around the corner.

This revision now requires changes to proceed.Dec 2 2018, 3:04 PM
Stan commandeered this revision.Dec 2 2018, 8:43 PM
Stan updated this revision to Diff 7022.
Stan added a reviewer: wraitii.

Getting this right doesn't seem like it would hurt the A23 re release was it to be included, right @Itms ?

Stan retitled this revision from Target 10.8 explicitly for Xcode to Target build version explicitly for Xcode.Dec 2 2018, 8:46 PM
Vulcan added a comment.Dec 2 2018, 8:48 PM

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

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

Itms requested changes to this revision.Dec 2 2018, 9:11 PM

I agree, but I think you didn't test this? The option must be accessed with _OPTIONS (see the other use of it in the file for an example) and I suggest running the whole premake script with and without the parameter passed.

This revision now requires changes to proceed.Dec 2 2018, 9:11 PM
Stan planned changes to this revision.Dec 2 2018, 9:25 PM

I assumed we should use premake variable because options might be undefined.

-- On OS X, sometimes we need to specify the minimum API version to use
			if _OPTIONS["macosx-version-min"] then
				buildoptions { "-mmacosx-version-min=" .. _OPTIONS["macosx-version-min"] }
				-- clang and llvm-gcc look at mmacosx-version-min to determine link target
				-- and CRT version, and use it to set the macosx_version_min linker flag
				linkoptions { "-mmacosx-version-min=" .. _OPTIONS["macosx-version-min"] }
			end
Stan updated this revision to Diff 7026.Dec 2 2018, 10:40 PM

Use the correct option, check if it is set before using it.

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

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

Itms requested changes to this revision.Dec 3 2018, 11:56 AM

The patch is currently useless, because xcodebuildsettings was added in premake5 alpha12 (we have alpha10): see https://github.com/premake/premake-core/wiki/xcodebuildsettings.

So this should be bumped to A24. At that point, D1685 will be in, so this patch should be rebased and pass --macosx-version-min="${MIN_OSX_VERSION}" to premake when generating the xcode workspaces.

This revision now requires changes to proceed.Dec 3 2018, 11:56 AM
Stan added a comment.Jan 1 2019, 10:56 AM

@wraitii care to commandeer it back ?

Stan updated this revision to Diff 7518.Mar 6 2019, 3:01 PM
  • Update. No need to rebase apparently.
Vulcan added a comment.Mar 6 2019, 3:04 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1081/display/redirect

Stan updated this revision to Diff 7520.Mar 6 2019, 3:05 PM
  • Just noticed it was already passed in the update workspaces
Itms accepted this revision.Apr 7 2019, 7:31 PM
Itms added a reviewer: macOS Developers.

This looks good to me now. Can somebody test on XCode (I don't have it on my VMs) and commit if it works as expected?

wraitii commandeered this revision.Apr 13 2019, 10:44 AM
wraitii edited reviewers, added: Stan; removed: wraitii.

Commandeering for minute adjustments.

wraitii updated this revision to Diff 7742.Apr 13 2019, 10:45 AM

I suggest setting 10.12 as the new min version (but that can be overturned), and Xcode could use setting the min OSX version too.

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1210/display/redirect

Itms requested changes to this revision.Apr 13 2019, 11:16 AM

Hi Lancelot ๐Ÿ™‚ Let's not change the minimum version, we worked hard enough during the rerelease to make sure we could keep 10.9 as the minimum. Apart from that, if it works for you I can accept.

This revision now requires changes to proceed.Apr 13 2019, 11:16 AM
wraitii updated this revision to Diff 7770.Apr 19 2019, 11:45 AM

Do not change minimal OSX version

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1227/display/redirect

Stan resigned from this revision.Apr 23 2019, 2:54 PM

I can't really review my own patch ^^

In D1482#75871, @Stan wrote:

I can't really review my own patch ^^

But you can review the changes of the guy that commandeered.

Stan added a comment.Apr 23 2019, 3:40 PM
In D1482#75878, @Imarok wrote:
In D1482#75871, @Stan wrote:

I can't really review my own patch ^^

But you can review the changes of the guy that commandeered.

Nothing changed :D

In D1482#75880, @Stan wrote:
In D1482#75878, @Imarok wrote:
In D1482#75871, @Stan wrote:

I can't really review my own patch ^^

But you can review the changes of the guy that commandeered.

Nothing changed :D

Then I don't see, why he has commandeered it...

Stan added a comment.Apr 23 2019, 7:37 PM

He made some changes then reverted them on Itms demand :)

I think I also changed build/workspaces/update-workspaces.sh

This revision was not accepted when it landed; it landed in state Needs Review.May 25 2019, 10:29 AM
This revision was automatically updated to reflect the committed changes.