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

iOS build broken due to std::system usage #2248

Closed
saurik opened this issue Apr 24, 2021 · 7 comments
Closed

iOS build broken due to std::system usage #2248

saurik opened this issue Apr 24, 2021 · 7 comments

Comments

@saurik
Copy link

saurik commented Apr 24, 2021

In 15c10b0, a new function was added--say--which runs std::system... a function which is "unavailable" on iOS, breaking the build of this library on that platform.

In file included from vpn/shared/p2p/fmt/src/os.cc:13:
./vpn/shared/p2p/fmt/include/fmt/os.h:205:8: error: 'system' is unavailable: not available on iOS
  std::system(format("say \"{}\"", format(format_str, args...)).c_str());
       ^
/Applications/Xcode_12.4.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS14.4.sdk/usr/include/stdlib.h:190:6: note: 'system' has been explicitly marked unavailable here
int      system(const char *) __DARWIN_ALIAS_C(system);
         ^
1 error generated.
make: *** [out-ios/arm64/./vpn/shared/p2p/fmt/src/os.cc.o] Error 1

I believe the correct check to remove this would be #ifndef TARGET_OS_IPHONE.

vitaut added a commit that referenced this issue Apr 24, 2021
@vitaut
Copy link
Contributor

vitaut commented Apr 24, 2021

Thanks for reporting. Does 553022d fix the problem?

@saurik
Copy link
Author

saurik commented Apr 25, 2021

Ugh... so, I've managed to make two mistakes in my attempt to guide you with "the correct check" :(.

  1. TARGET_OS_IPHONE is actually set to either 1 or 0 and so you can't use #ifndef and must use #if !.
  2. TARGET_OS_IPHONE is not defined in this context unless you also #include <TargetConditionals.h>.

I've tested that this patch works:

diff --git a/include/fmt/os.h b/include/fmt/os.h
index 70c465b5..5955339e 100644
--- a/include/fmt/os.h
+++ b/include/fmt/os.h
@@ -14,6 +14,10 @@
 #include <cstdio>
 #include <cstdlib>  // for strtod_l

+#if defined __APPLE__
+#  include <TargetConditionals.h>
+#endif
+
 #if defined __APPLE__ || defined(__FreeBSD__)
 #  include <xlocale.h>  // for LC_NUMERIC_MASK on OS X
 #endif
@@ -201,7 +205,7 @@ FMT_API void report_windows_error(int error_code,
 #endif  // _WIN32

 // std::system is not available on iOS (#2248).
-#ifndef TARGET_OS_IPHONE
+#if !TARGET_OS_IPHONE
 template <typename S, typename... Args, typename Char = char_t<S>>
 void say(const S& format_str, Args&&... args) {
   std::system(format("say \"{}\"", format(format_str, args...)).c_str());

Another option is to use __IPHONE_OS_VERSION_MIN_REQUIRED, which is defined by the compiler itself, but the problem is that tvOS and watchOS are going to have the same issue and while those platforms usually also have the __IPHONE_OS_VERSION_MIN_REQUIRED set, I believe that is just some backwards-compatibility in AvailabilityInternal.h and I have no clue if Apple intends to maintain that...

@vitaut
Copy link
Contributor

vitaut commented Apr 25, 2021

Thinking more of it, it's better to opt in instead of opt out platforms, so here's another tentative fix: ce67532.

@saurik
Copy link
Author

saurik commented Apr 26, 2021

Ah, of course: since that say command only exists on macOS in the first place. (Why, BTW, is fmt providing an OSX-specific text-to-speech function?... it isn't even "safe": if the string contains a quotation mark it can terminate the command and obtain arbitrary shell execution :(.) FWIW, this patch at least made fmt compile on iOS, if it is working for your use cases!

@bbolli
Copy link
Contributor

bbolli commented Apr 26, 2021

Why, BTW, is fmt providing an OSX-specific text-to-speech function?

@saurik, check the date of 15c10b0...

@saurik
Copy link
Author

saurik commented Apr 26, 2021

"lol" :(

@vitaut
Copy link
Contributor

vitaut commented Apr 26, 2021

Fixed in 69bdc20.

@vitaut vitaut closed this as completed Apr 26, 2021
@vitaut vitaut changed the title iOS build broken due to sys::system usage iOS build broken due to std::system usage Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants