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 OpenSSL versions 1.1.1 & newer for tvOS #1586

Merged
merged 2 commits into from
Jun 10, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions recipes/openssl/1.x.x/conanfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ class OpenSSLConan(ConanFile):
default_options["openssldir"] = None
_env_build = None
_source_subfolder = "source_subfolder"
exports_sources = ['patches/*']

def build_requirements(self):
if tools.os_info.is_windows:
Expand Down Expand Up @@ -413,6 +414,8 @@ def _configure_args(self):
if self._full_version >= "1.1.0":
args.append("--debug" if self.settings.build_type == "Debug" else "--release")

if str(self.settings.os) == "tvOS":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if str(self.settings.os) == "tvOS":
if self.settings.os == "tvOS":

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is changed, then the rest of the if self.settings.os == statements that follow should also be changed. I didn't notice it before, but the usage of this is horribly inconsistent though the entire recipe. Is there a standardization somewhere that says one should be used over the other? I typically don't do "standardization" type changes for commits for things like this, but I'm happy to do it if you'd prefer it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By dropping the str, conan is able to check whether tvOS is a valid os.
That way typos can be catched.
If the code is equivalent, I'ld say 'why not?'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it doesn't matter to me one way or the other - I suck at Python & honestly don't know the difference.

Just because I'm curious and would like to know - how is it that Conan can check if it's a valid OS by doing self.settings.os rather than str(self.settings.os)? If self.settings.os is a string, then using str() really just turns an existing string into a string, right?

Maybe a better way to put it is - how is it that dropping str() makes it so Conan can check if tvOS is a valid OS?

Copy link
Contributor

@madebr madebr May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you compare against self.settings.os, you're comparing against a SettingsItem object.
This class checks whether strings to which it is compared against would also be a valid setting (ie it is present in your settings.yml file).
By doing str, you compare against a string without an additional check.

args.append(" -DNO_FORK") # fork is not available on tvOS
if str(self.settings.os) == "Android":
args.append(" -D__ANDROID_API__=%s" % str(self.settings.os.api_level)) # see NOTES.ANDROID
if str(self.settings.os) == "Emscripten":
Expand Down Expand Up @@ -603,6 +606,9 @@ def build(self):
env_vars["CROSS_TOP"] = os.path.dirname(os.path.dirname(xcrun.sdk_path))
with tools.environment_append(env_vars):
if self._full_version >= "1.1.0":
if self.settings.os == "tvOS":
tools.patch(patch_file=os.path.join("patches", "1.1.1-tvos.patch"),
base_path=self._source_subfolder)
Comment on lines +609 to +611
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the conditional patching.
Can the patch be rewritten so it only changes behavior for WatchOS?
Maybe by checking for the existence for a certain preprocessor variable that can be set in the conanfile.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering this is the patch that is being used as the fix for this in the official OpenSSL repository, I don't think that makes sense. Furthermore, since even if you pass -DNO_FORK, it gets undefined in apps/ocsp.c, that is a bug in OpenSSL itself that is not specific to tvOS, so I would argue that it shouldn't be made specific to tvOS.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Good point.

self._create_targets()
else:
self._patch_makefile_org()
Expand Down
48 changes: 48 additions & 0 deletions recipes/openssl/1.x.x/patches/1.1.1-tvos.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
--- apps/speed.c
+++ apps/speed.c
@@ -99,6 +99,13 @@
#endif
#include <openssl/modes.h>

+/* fork() breaks AppleTVOS, WatchOS, AppleTVSimulator and WatchSimulator */
+/* Users should configure with -DNO_FORK */
+#if defined(NO_FORK)
+# undef HAVE_FORK
+# define HAVE_FORK 0
+#endif
+
#ifndef HAVE_FORK
# if defined(OPENSSL_SYS_VMS) || defined(OPENSSL_SYS_WINDOWS) || defined(OPENSSL_SYS_VXWORKS)
# define HAVE_FORK 0
@@ -110,6 +117,7 @@
#if HAVE_FORK
# undef NO_FORK
#else
+# undef NO_FORK
# define NO_FORK
#endif

--- apps/ocsp.c
+++ apps/ocsp.c
@@ -36,6 +36,13 @@
# include <openssl/x509v3.h>
# include <openssl/rand.h>

+/* fork() breaks AppleTVOS, WatchOS, AppleTVSimulator and WatchSimulator */
+/* Users should configure with -DNO_FORK */
+#if defined(NO_FORK)
+# undef HAVE_FORK
+# define HAVE_FORK 0
+#endif
+
#ifndef HAVE_FORK
# if defined(OPENSSL_SYS_VMS) || defined(OPENSSL_SYS_WINDOWS)
# define HAVE_FORK 0
@@ -47,6 +54,7 @@
#if HAVE_FORK
# undef NO_FORK
#else
+# undef NO_FORK
# define NO_FORK
#endif