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

CREATE-PROJECT: Various portability fixes for Mac OS X Leopard and other older systems #2751

Conversation

@dwatteau
Copy link
Contributor

@dwatteau dwatteau commented Feb 3, 2021

Here's a set of changes I had to make so that create_project could be built on Mac OS X Leopard.

In summary:

  • pick the override compat macro from c++11-compat.h to msbuild.h, because this file makes use of the override keyword, and older compilers don't have it.
  • use <CommonCrypto/CommonDigest.h>, not <CommonCrypto/CommonCrypto.h> for CC_MD5(). This is what the manpage documents, and Leopard will not find it without the proper header.
  • use the older POSIX.1-2001 syntax for realpath(), not the POSIX.1-2008 one, because it will cause a crash on older systems. Don't free(rp); anymore, since we're no longer using the heap for this.

This is maybe useless (although the one for CommonDigest.h still looks good to me), or you may want to do this a bit differently, but here's my proposal :)

Tested on Leopard and Mojave.

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Feb 4, 2021

Maybe move the override workaround to create_project.h so that it's applied everywhere, otherwise using override in the future in a different file will break this again.

These changes look good to me, thanks! I appreciate create_project building in old environments.

@dwatteau dwatteau force-pushed the dwatteau:fix/improve-portability-with-older-macos-releases branch from 5346e82 to a6d7326 Feb 8, 2021
@dwatteau
Copy link
Contributor Author

@dwatteau dwatteau commented Feb 8, 2021

Maybe move the override workaround to create_project.h so that it's applied everywhere, otherwise using override in the future in a different file will break this again.

These changes look good to me, thanks! I appreciate create_project building in old environments.

Thank you! I've edited the PR with your suggestion.

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Feb 9, 2021

xcode.cpp was just updated in master and now there's a conflict, can you please rebase this against master?

dwatteau added 3 commits Feb 3, 2021
<CommonCrypto/CommonDigest.h> should be used for CC_MD5() on macOS.

Fixes the build on Leopard.
Non POSIX.1-2008 systems will not necessarily accept NULL as the
second argument, and could crash the program here. Provide a
temporary buffer on the stack instead, and don't count on realpath()
allocating one on the heap. This will work on older POSIX.1-2001
systems.

(In theory, this older POSIX.1-2001 syntax could have portability
problems too, because of PATH_MAX, but in practice this shouldn't
be a problem for the systems intended to be used by create_project.)

Fixes the build on Mac OS X Leopard.
msbuild.h makes uses of the "override" keyword, so borrow the compat
macro from c++11-compat.h to make this work on older compilers,
such as the ones found on Mac OS X Leopard.
@dwatteau dwatteau force-pushed the dwatteau:fix/improve-portability-with-older-macos-releases branch from a6d7326 to 05c292f Feb 9, 2021
@dwatteau
Copy link
Contributor Author

@dwatteau dwatteau commented Feb 9, 2021

xcode.cpp was just updated in master and now there's a conflict, can you please rebase this against master?

Ah yeah, I had a conflict with one of my own commits there :)

It should be OK now.

@sluicebox
Copy link
Member

@sluicebox sluicebox commented Feb 9, 2021

I've tested this on Big Sur (M1) and Catalina (Intel) and each change makes sense. Nice job!

@sluicebox sluicebox merged commit 5a78e13 into scummvm:master Feb 9, 2021
2 of 3 checks passed
2 of 3 checks passed
deepcode-ci-bot DeepCode is checking for issues.
Codacy Static Code Analysis Codacy Static Code Analysis
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants