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.

Sign up for GitHub

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 issue: wrong Darwin value for iOS #2846

Closed
wants to merge 5 commits into from
Closed

fix issue: wrong Darwin value for iOS #2846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
be1595b
fix issue: wrong Darwin value for iOS; CMake MATCHES v.s STREQUAL is …
kgbook Jan 8, 2024
26ce98a
fixed issue:cmake CMP0153 policy issue, exec_program is deprecated an…
kgbook Jan 9, 2024
bc6626f
fixed issue: cmake policy CMP0054 issue, CMake version 3.28.1 warns w…
kgbook Jan 9, 2024
57d1108
fixed issue: Compatibility with CMake < 3.5 will be removed from a fu…
kgbook Jan 9, 2024
2343e3e
fixed issue: No iOS SDK's found in default search path, wrong value,…
kgbook Jan 9, 2024
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
14 changes: 9 additions & 5 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
# file, You can obtain one at http://mozilla.org/MPL/2.0/.
#

cmake_minimum_required (VERSION 2.8.12 FATAL_ERROR)
cmake_minimum_required (VERSION 3.5 FATAL_ERROR)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can't raise the minimum required CMake version in a patch release1.5.4 scheduled to be the next one.
In any case there should be a strong motivation for raising this requirement, as it affects portability of SRT a bit (requires additional effort to update CMake on some older platforms).

set (SRT_VERSION 1.5.3)

set (CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/scripts")
Expand All @@ -24,17 +24,21 @@ else()
project(SRT VERSION ${SRT_VERSION} LANGUAGES C CXX)
endif()

if (${CMAKE_VERSION} VERSION_GREATER_EQUAL 3.1)
cmake_policy(SET CMP0054 NEW)
endif ()

include(FindPkgConfig)
# XXX See 'if (MINGW)' condition below, may need fixing.
include(FindThreads)
include(CheckFunctionExists)

# Platform shortcuts
string(TOLOWER ${CMAKE_SYSTEM_NAME} SYSNAME_LC)
set_if(DARWIN (${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
OR (${CMAKE_SYSTEM_NAME} MATCHES "iOS")
OR (${CMAKE_SYSTEM_NAME} MATCHES "tvOS")
OR (${CMAKE_SYSTEM_NAME} MATCHES "watchOS"))
set_if(DARWIN (CMAKE_SYSTEM_NAME MATCHES "Darwin")
OR (CMAKE_SYSTEM_NAME MATCHES "iOS")
OR (CMAKE_SYSTEM_NAME MATCHES "tvOS")
OR (CMAKE_SYSTEM_NAME MATCHES "watchOS"))
set_if(LINUX ${CMAKE_SYSTEM_NAME} MATCHES "Linux")
set_if(BSD ${SYSNAME_LC} MATCHES "bsd$")
set_if(MICROSOFT WIN32 AND (NOT MINGW AND NOT CYGWIN))
Comment on lines +38 to 44
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am always confused with those ${var} and var notations in CMake. With this change you are passing the variable CMAKE_SYSTEM_NAME itself instead of its value to the MATCHES comparator. What's the difference with passing the value?
Also following set_if statements are using a different notation to what is proposed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

CMake documentation defines it this way:

if(<variable|string> MATCHES <regex>)

I'm not sure how CMake handles it in every single case, however by using ${var} you risk that if this resolves to an empty string, you effectively cause a syntax error. OTOH I have no idea what exactly happens if it resolves to a name of an existing variable. Probably the direct string and an effective string resolved from a variable are also distinguished. Not sure how it exactly works provided that the conditional statement is simply placed as argument for if:

MACRO(set_if varname)
	IF(${ARGN})
		SET(${varname} 1)
	ELSE(${ARGN})
		SET(${varname} 0)
	ENDIF(${ARGN})
ENDMACRO(set_if)

Anyway, if you are certain that a particular variable exists, you can use just as well ${CMAKE_SYSTEM_NAME}.

Frankly I don't understand, what this change is here and actually when I compare it to the latest version, it just changes ${var} into var. This change is completely unnecessary. I understand the problem was with wrong values being compared against. This just solves the problem of a case when CMAKE_SYSTEM_NAME variable doesn't exist or contains an empty string. But they write here that it's not the case.

Expand Down
11 changes: 9 additions & 2 deletions scripts/iOS.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ set (CMAKE_OSX_DEPLOYMENT_TARGET "" CACHE STRING "Force unset of the deployment
# Determine the cmake host system version so we know where to find the iOS SDKs
find_program (CMAKE_UNAME uname /bin /usr/bin /usr/local/bin)
if (CMAKE_UNAME)
exec_program(uname ARGS -r OUTPUT_VARIABLE CMAKE_HOST_SYSTEM_VERSION)
execute_process(
COMMAND uname -r
OUTPUT_VARIABLE CMAKE_HOST_SYSTEM_VERSION
)
string (REGEX REPLACE "^([0-9]+)\\.([0-9]+).*$" "\\1" DARWIN_MAJOR_VERSION "${CMAKE_HOST_SYSTEM_VERSION}")
endif (CMAKE_UNAME)

Expand Down Expand Up @@ -119,7 +122,11 @@ endif (${IOS_PLATFORM} STREQUAL OS)

# Setup iOS developer location unless specified manually with CMAKE_IOS_DEVELOPER_ROOT
if (NOT DEFINED CMAKE_IOS_DEVELOPER_ROOT)
exec_program(/usr/bin/xcode-select ARGS -print-path OUTPUT_VARIABLE CMAKE_XCODE_DEVELOPER_DIR)
execute_process(
COMMAND /usr/bin/xcode-select -print-path
OUTPUT_VARIABLE CMAKE_XCODE_DEVELOPER_DIR
)
string(STRIP "${CMAKE_XCODE_DEVELOPER_DIR}" CMAKE_XCODE_DEVELOPER_DIR) # FIXED: remove new line character
set (CMAKE_IOS_DEVELOPER_ROOT "${CMAKE_XCODE_DEVELOPER_DIR}/Platforms/${IOS_PLATFORM_LOCATION}/Developer")
endif (NOT DEFINED CMAKE_IOS_DEVELOPER_ROOT)
set (CMAKE_IOS_DEVELOPER_ROOT ${CMAKE_IOS_DEVELOPER_ROOT} CACHE PATH "Location of iOS Platform")
Expand Down
Loading