Skip to content
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

Fix Mac builds #318

Open
wants to merge 4 commits into
base: master
from
Open

Fix Mac builds #318

wants to merge 4 commits into from

Conversation

@JensAyton
Copy link
Member

JensAyton commented Jan 13, 2019

These are the changes required to build with Xcode 10, which will bring back nightly builds. It also updates various project files, breaking compatibility with older versions of Xcode.

Done as a PR because merging this will drop support for Mac OS X 10.6 (the 2009–2010 version).

@KonstantinosSykas

This comment has been minimized.

Copy link
Contributor

KonstantinosSykas commented Jan 14, 2019

Merging this PR will most probably leave macOS out of future releases, until another maintainer with Xcode10 enabled environment joins the team.
EDIT 2019-01-15 11:50 EET (GMT +02:00): Xcode 10 requires a Mac running macOS 10.13.6 or later.

For the time being, Travis CI compiling AI.m and OOTrumble.m fails, complaining about OOViewID.tbl file not found.
Could it be the version of Xcode used by Travis CI?

Our .travis.yml file tells Travis CI to use Xcode 7.3 and my understanding is that this PR's changes break compatibility with Xcode versions less than 10.

language: objective-c
osx_image: xcode7.3

xcode_project: Oolite.xcodeproj
xcode_scheme: 'Oolite - Test Release'

Could you update this PR with a .travis.yml having a compatible (with the changes committed) osx_image value?
For the list of valid osx_image values you may check the relevant Travis CI reference page here.

@phkb

This comment has been minimized.

Copy link
Contributor

phkb commented Jan 20, 2019

Not sure why the HeadUpDisplay.m and OOConstToJSString.m files have been included in this PR. They seem to be removing things that are unrelated to any Mac-specific code. Accidental, perhaps?

@KonstantinosSykas

This comment has been minimized.

Copy link
Contributor

KonstantinosSykas commented Jan 21, 2019

@phkb
Both fixes deal with compilation warnings on Mac. In Linux and Windows, these warnings did not appear due to different compiler options used.

@phkb

This comment has been minimized.

Copy link
Contributor

phkb commented Jan 21, 2019

Ah, got it. It's still a little confusing, though, because there doesn't appear to be any difference between the lines being removed and similar ones around them. For instance, why would the Mac have a problem with lines 119-123 of OOConstToJSString.m, when they're almost identical to lines 114-117? Why do those lines compile, but lines 119-123 don't?

It's also probably worth noting that, by applying the changes to OOConstToJSString.m, commit 835770b would probably need to be reverted as that change relies on the new table.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.