Set correct minimum macOS version with CMake

[Kolcha]

Info.plist has ${MACOSX_DEPLOYMENT_TARGET} placeholder which is filled
by qmake, but not CMake. And as result, when building with CMake value
of LSMinimumSystemVersion field is '.0'. This happens because CMake has
possibility to substitute placeholders with values from corresponding
variables, and as so as there is no such variable, empty value is used.

So, to fix this, just set CMake variable MACOSX_DEPLOYMENT_TARGET to
CMAKE_OSX_DEPLOYMENT_TARGET value.

no fix, master as is:

with fix, this branch:

P.S> maybe it is worth to set CMAKE_OSX_DEPLOYMENT_TARGET explicitly in root CMakeLists.txt instead of passing it to command line each time (especially when 10.14 is required)? similar thing is done for qmake:

qBittorrent/macxconf.pri

Line 10 in c7c7924

QMAKE_MACOSX_DEPLOYMENT_TARGET = 10.14

[FranciscoPombal]

I don't think this is the right approach.

See the docs: https://cmake.org/cmake/help/latest/variable/CMAKE_OSX_DEPLOYMENT_TARGET.html

MACOSX_DEPLOYMENT_TARGET is supposed to be an environment variable. As such, it should not be forced in the build script.

According to the docs, it's CMAKE_OSX_DEPLOYMENT_TARGET that is supposed to be initialized by MACOSX_DEPLOYMENT_TARGET, not the other way around.

Also, according to the docs, this is also the wrong place to set it:

The value of this variable should be set prior to the first project() or enable_language() command invocation because it may influence configuration of the toolchain and flags. It is intended to be set locally by the user creating a build tree. This variable should be set as a CACHE entry (or else CMake may remove it while initializing a cache entry of the same name).

In addition, the fact that currently ${MACOSX_DEPLOYMENT_TARGET} is currently used as the variable name for the substitution in the Info.plist file is a problem IMO, so the fix should start by changing that probably.

Let's hope qmake does not conflict with "the right way" to do things here. Ah, the joys of maintaining 2 build systems...

[FranciscoPombal]

@Kolcha

I think this should work (sorry for not posting in actual patch format, doing this in a hurry; the explanation for each change is at the end of the post):

1. In src/app/CMakeLists.txt

change this:

    # substitute @EXECUTABLE@ in dist/mac/Info.plist
    get_target_property(EXECUTABLE qbt_app OUTPUT_NAME)
    configure_file(${qBittorrent_SOURCE_DIR}/dist/mac/Info.plist
        ${qBittorrent_BINARY_DIR}/dist/mac/pregen.plist @ONLY)
    file(GENERATE
        OUTPUT ${qBittorrent_BINARY_DIR}/dist/mac/Info.plist
        INPUT ${qBittorrent_BINARY_DIR}/dist/mac/Info.plist
    )

to this:

    # perform @ substitutions
    get_target_property(EXECUTABLE qbt_app OUTPUT_NAME)
    configure_file(${qBittorrent_SOURCE_DIR}/dist/mac/Info.plist
        ${qBittorrent_BINARY_DIR}/dist/mac/pregen.plist @ONLY)

2. In dist/mac/Info.plist

change this:

<string>${MACOSX_DEPLOYMENT_TARGET}.0</string>

to this:

<string>@CMAKE_OSX_DEPLOYMENT_TARGET@</string>

Basically, I don't think we need the file(GENERATE) step at all, since we don't have any generator expressions in the file. So we just have to change the one variable substitution related to the deployment target to an @ substitution.

IDK what changes would be required on the qmake side though. If the naming of the substitution is a problem for qmake, you could do something like set(SOME_VAR_NAME "${CMAKE_OSX_DEPLOYMENT_TARGET}") before the call to configure_file()and put @SOME_VAR_NAME@ in the info.plist file instead of @CMAKE_OSX_DEPLOYMENT_TARGET@, where SOME_VAR_NAME is something that qmake is also happy about, and that is not MACOSX_DEPLOYMENT_TARGET, for the reasons explained in my comment above.

[Kolcha]

@FranciscoPombal thanks for such good review! I waited it.
I'll try to follow your suggestions in few days, unfortunately my MacBook is unusable now... I have to reinstall macOS and setup development environment... it will take pretty much time

[Kolcha]

so, I'm back.
I started from investigation what qmake can and tried to replace ${VAR} with @VAR@ in case of macOS version.
so, result was expected to me - qmake DIDN'T replace anything... I opened Qt documentation about supported variables in Info.plist, and was surprised to read that ${VAR} style is used for Xcode generator... but in case of Makefile generator such variables are actually replaced with sed (as @VAR@ style variables), there is line I found in generated Makefile:

@sed -e "s,@SHORT_VERSION@,1.0,g" -e "s,\$${QMAKE_SHORT_VERSION},1.0,g" -e "s,@FULL_VERSION@,1.0.0,g" -e "s,\$${QMAKE_FULL_VERSION},1.0.0,g" -e "s,@TYPEINFO@,????,g" -e "s,\$${QMAKE_PKGINFO_TYPEINFO},????,g" -e "s,@BUNDLEIDENTIFIER@,com.yourcompany.qbittorrent,g" -e "s,\$${PRODUCT_BUNDLE_IDENTIFIER},com.yourcompany.qbittorrent,g" -e "s,\$${MACOSX_DEPLOYMENT_TARGET},10.14,g" -e "s,\$${IPHONEOS_DEPLOYMENT_TARGET},,g" -e "s,\$${TVOS_DEPLOYMENT_TARGET},,g" -e "s,\$${WATCHOS_DEPLOYMENT_TARGET},,g" -e "s,@ICON@,qbittorrent_mac.icns,g" -e "s,\$${ASSETCATALOG_COMPILER_APPICON_NAME},qbittorrent_mac.icns,g" -e "s,@EXECUTABLE@,qbittorrent,g" -e "s,@LIBRARY@,qbittorrent,g" -e "s,\$${EXECUTABLE_NAME},qbittorrent,g" -e "s,@TYPEINFO@,????,g" -e "s,\$${QMAKE_PKGINFO_TYPEINFO},????,g" ../../qBittorrent/dist/mac/Info.plist >./qbittorrent.app/Contents/Info.plist

that's how qmake actually "sets" these variables. also I made some search for any solutions how to make variable substitutions if files using qmake, unfortunately there is no such API... one solution I see that read whole file into variable, make string substitutions, and write it again using qmake functions. but I think such inconvenient approach is not worth this small issue...

from other side, cmake also doesn't provide any "right way" to "deliver" this particular value to Info.plist. what I did just works due to cmake feature mentioned in last sentences of that page.

so, now I can suggest next solutions for it:

1. leave my proposed change as is. I understand that this is a hack (and slightly confusing due to reasons in first comment), but it is safe because environment variable != cmake variable, and cmake variable with the same name as environment variable may coexists because cmake can't read or write environment variables without special syntax and that expression is not in Makefile where it can affect build process somehow
2. just hardcode required value and maintain its updates
3. just drop LSMinimumSystemVersion key from Info.plist . unfortunately, now I can't find in Apple documentation tables with a lot of keys and their description (also with required and recommended marks) what I saw long time ago (but found pages describing each key separately), but IIRC this key was not marked as required. moreover, when no CMAKE_OSX_DEPLOYMENT_TARGET neither corresponding environment variable is set, empty value is also passed to Info.plist... it has no default value (aka maximum available based on used SDK, what is actually happens with binary in such case)

I'm fine with any of suggested options, but first two are preferable for me. any thoughts and criticism are welcome.

[FranciscoPombal]

@Kolcha

There may be a simpler solution: can't we get away with simply changing every @VAR@ substitution to a ${VAR} substitution in the Info.plist template?

Then you could apply the changes suggested in my comment, and just remove the @ONLY argument from the call to configure_file().

That way, there would be no problem with qmake.

---

If this is not possible, then I would personally prefer solution 2) that you suggested. A bunch of other stuff is also hardcoded, and it's not like we would have to update that field too often...

3. would be the simplest, but if I understand correctly, the compiled binary will only run on systems at least as recent as the most recent version that the SDK (with which qBittorrent is compiled) supports? That is clearly not desirable, so that option is ruled out.

[Kolcha]

@FranciscoPombal
yes, we can use ${VAR} for all variables in Info.plist https://doc.qt.io/qt-5/qmake-variable-reference.html#qmake-info-plist
but in such case variables names should be known to qmake, i.e. MACOSX_DEPLOYMENT_TARGET you don't like (and I understand why)
even more, in such case configure_file() will not be required at all - cmake will make substitution on copy (tested)

[Kolcha]

@FranciscoPombal , PR is updated. I used ${VAR} variables style in Info.plist, dropped configure_file() and file(GENERATE) calls, just set 2 cmake variables for corresponding variables in Info.plist.
both build systems produced exactly the same Info.plist in .app bundle.
MACOSX_DEPLOYMENT_TARGET variable is left because qmake doesn't understand anything else except the set of predefined variables listed in docs

[FranciscoPombal]

OK, seems reasonable, just 2 more comments based on the new changes.

[FranciscoPombal]

LGTM now, thanks @Kolcha!

[Chocobo1]

@Kolcha
Thank you!