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

add support for CMAKE_OSX_ARCHITECTURES when detecting SIZEOF variables #3945

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@JonasVautherin
Copy link
Contributor

commented May 25, 2019

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

@JonasVautherin

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented May 27, 2019

@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

This comment has been minimized.

Copy link
Member

commented May 27, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

@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

This comment has been minimized.

Copy link
Member

commented May 27, 2019

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

@jzakrzewski

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

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

This comment has been minimized.

Copy link
Member

commented May 27, 2019

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

@JonasVautherin

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

For me personally that'd be enough.

@snikulov

This comment has been minimized.

Copy link
Member

commented May 28, 2019

LGTM.

@bagder

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Thanks!

@bagder bagder closed this in 5aa2347 May 28, 2019

@JonasVautherin JonasVautherin deleted the JonasVautherin:support-osx-architectures branch May 28, 2019

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