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

Jump to bottom

add support for CMAKE_OSX_ARCHITECTURES when detecting SIZEOF variables #3945

Closed
wants to merge 1 commit into from
Closed

add support for CMAKE_OSX_ARCHITECTURES when detecting SIZEOF variables #3945

wants to merge 1 commit into from

Conversation

JonasVautherin
Copy link
Contributor

When cross-compiling for iOS, toolchains set CMAKE_OSX_ARCHITECTURES. As per the doc:

The variable “${VARIABLE}” may be “0” when CMAKE_OSX_ARCHITECTURES has multiple architectures for building OS X universal binaries. This indicates that the type size varies across architectures.

In this case, e.g. ${SIZEOF_LONG} gets set to "0", and therefore #cmakedefine SIZEOF_LONG ${SIZEOF_LONG} becomes "/* #undef SIZEOF_LONG */" in curl_config.h, resulting in a broken build.

However, and still described in the docs, when ${SIZEOF_LONG} is "0", CMake also defines other values, including ${SIZEOF_LONG_CODE}, which gives, e.g. for CMAKE_OSX_ARCHITECTURES=armv7;armv7s;arm64:

#if defined(__ARM_ARCH_7A__)
# define SIZEOF_LONG 4
#elif defined(__ARM_ARCH_7S__)
# define SIZEOF_LONG 4
#elif defined(__aarch64__)
# define SIZEOF_LONG 8
#else
# error SIZEOF_LONG unknown
#endif

Instead of using #cmakedefine and ${SIZEOF_LONG}, I believe it always works to use ${SIZEOF_LONG_CODE} directly, while enabling support for multiple architectures in CMAKE_OSX_ARCHITECTURES.

It is working for me on Linux, and on macos when cross-compiling for iOS (armv7;armv7s;arm64).

@bagder bagder added the cmake label May 25, 2019
@bagder bagder requested a review from snikulov May 25, 2019 21:26
@JonasVautherin
Copy link
Contributor Author

I don't get how my changes could reduce the coverage by 8%. And the other error seems to be a timeout in the CI.

@bagder
Copy link
Member

bagder commented May 26, 2019

@JonasVautherin no, you're right. The coveralls service acts up like this every now and then, so in this particular case we'll just ignore it. And the cirrus-ci freebsd test sometimes times out and that's another one to ignore...

@snikulov
Copy link
Member

@JonasVautherin pardon me... But why #cmakedefine directive was removed?
Was it some syntax change in CMake?
Could you please explain how it works now according to documentation?

@snikulov
Copy link
Member

snikulov commented May 27, 2019 •
edited

@JonasVautherin After some investigation, I've discovered your point.
While it seems to be working, it's not clear for understanding.
No any direct references from configure_file to CheckTypeSize provided.

@jzakrzewski
Copy link
Contributor

@snikulov surprised me also but CheckTypeSize documentation makes it clear now. Nevertheless, it feels pretty disconnected but I don't know if there's a way to make it easier to read.

@snikulov
Copy link
Member

@jzakrzewski perhaps @bradking could shed some light on this.
@bradking Is it correct to use ${SIZEOF_LONG_CODE} instead of #cmakedefine?

@jzakrzewski
Copy link
Contributor

Judging from the documentation I''d say it is correct. But there's this "Where the heck is that coming from?" moment. I guess for an expert it may be obvious. I haven't written (yet) enough CMake to know it from the first glance.
Maybe a reminder-comment somewhere would suffice? Because this seems quite elegant once you know why it works.

@bagder
Copy link
Member

bagder commented May 27, 2019

Maybe an added comment with the link to the documentation for it is enough?

@JonasVautherin
Copy link
Contributor Author

JonasVautherin commented May 27, 2019 •
edited

I updated with a description before the SIZEOF_ section, I hope it makes it clearer. There is again a timeout in the CI (Cirrus), and I don't get why appveyor fails.

@jzakrzewski
Copy link
Contributor

For me personally that'd be enough.

@snikulov
Copy link
Member

LGTM.

@bagder
Copy link
Member

bagder commented May 28, 2019

Thanks!

@bagder bagder closed this in 5aa2347 May 28, 2019
@JonasVautherin JonasVautherin deleted the support-osx-architectures branch May 28, 2019 09:33
@lock lock bot locked as resolved and limited conversation to collaborators Aug 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@snikulov snikulov snikulov approved these changes

Assignees
No one assigned
Labels
cmake
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JonasVautherin @bagder @snikulov @jzakrzewski