[ffmpeg] Honor macosx deployment target for ffmpeg

[omartijn]

Ports that are not CMake-based, which includes most nominally ports using Autotools, ignore options like VCPKG_OSX_DEPLOYMENT_TARGET. This is not an issue for our normal triplets, since they don't set this flag anyway, but when used in manifest mode it's common to set this to the same deployment target as the software it's used in.

This PR makes ffmpeg honor this flag by setting the correct c- and linker flags.

• What does your PR fix?

Fixes ffmpeg ignoring VCPKG_MACOSX_DEPLOYMENT_TARGET

• Which triplets are supported/not supported? Have you updated the CI baseline?

No changes to triplets, ci baseline is not updated

• Does your PR follow the maintainer guide?

Yes

• If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Versions have been updated.

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

[Neumann-A]

I would consider this PR invalid.

The proper fix is to call vcpkg_internal_get_cmake_vars in the ffmpeg portfile and setup the environment correctly for the ffmpeg build with the given cmake variables. This should in return automatically set the correct deployment targets.

[omartijn]

I would consider this PR invalid.

The proper fix is to call vcpkg_internal_get_cmake_vars in the ffmpeg portfile and setup the environment correctly for the ffmpeg build with the given cmake variables. This should in return automatically set the correct deployment targets.

I disagree. The documentation for this function quite clearly states that it's meant only for internal use in vcpkg helpers, since behavior and arguments will change without notice. So you wouldn't want to put this in a portfile, since that means you'd have ports breaking all the time when a change in this function happens.

[Neumann-A]

I disagree. The documentation for this function quite clearly states that it's meant only for internal use in vcpkg helpers, since behavior and arguments will change without notice.

There are exceptions to 'rules' for ports with really bad build systems like ffmpeg which do not use vcpkg helpers due to being a extraordinary special kind of flower. As such creating an extra helper would be too port specific. (I mean you could create vcpkg_configure_ffmpeg and put it into the ffmpeg port .... but do you want that?)

[omartijn]

@Neumann-A Can you explain a little bit more about what the advantages are of this setup? I have tried doing this by adding the call to the vcpkg_internal_get_cmake_vars function and then including it, but that is not enough to get things going.

I do see that the VCPKG_DETECTED_CMAKE_OSX_DEPLOYMENT_TARGET is set (and then loaded) from the file. I can of course set this as an environment variable instead of passing it to configure, but I'm curious what exactly using the vcpkg_internal_get_cmake_vars gives us as an advantage over just using the VCPKG_OSX_DEPLOYMENT_TARGET instead.

[Neumann-A]

You need to add all the FLAGS env variables as done inininhttps://github.com/microsoft/vcpkg/blob/master/scripts/cmake/vcpkg_configure_make.cmake

[omartijn]

I still don't understand how that is advantageous. The file you're linking to seems to do things to process variables like VCPKG_DETECTED_CMAKE_C_FLAGS_DEBUG and its assorted variants, none of which have anything to do with osx deployment targets.

So, to implement your suggestion I would add a dependency on something marked internal, and then I can use VCPKG_DETECTED_CMAKE_OSX_DEPLOYMENT instead of VCPKG_OSX_DEPLOYMENT. How is that better than simply using VCPKG_OSX_DEPLOYMENT and not adding the dependency.

[Neumann-A]

I still don't understand how that is advantageous.

the bigger picture. ffmpeg is not correctly setting compiler and compiler/linker flags. in vcpkg everything should depend on the cmake_toolchain_file which ffmpeg is currently not.

The file you're linking to seems to do things to process variables like VCPKG_DETECTED_CMAKE_C_FLAGS_DEBUG and its assorted variants, none of which have anything to do with osx deployment targets.

wrong. these are the flags emitted by cmake if cmake is adding them somewhere else like e.g. in the platform scripts they need to be added to the detection script. this then not only fixes ffmpeg but all ports depending on the detection script.
All ports which do not use cmake need to call vcpkg_internal_get_cmake_vars to correctly setup compiler and linker flags or have a CMakeLists.txt forwarding the cmake vars to the ports buildsystem.

[PhoebeHui]

cc @mcmtroffaes @Sibras

[mcmtroffaes]

Unfortunately, I fear I'm not familiar with the internal workings of vcpkg_internal_get_cmake_vars to understand exactly what's needed here, and the documentation isn't particularly helpful. From what I gather from looking at existing code, the intended usage appears to be:

vcpkg_internal_get_cmake_vars(OUTPUT_FILE _VCPKG_CMAKE_VARS_FILE)
include("${_VCPKG_CMAKE_VARS_FILE}")

but as noted above, this isn't enough.

I agree that ideally non-standard ports should set linker and compiler flags similar to standard ports. Perhaps someone with a solid understanding of the inner workings of vcpkg could document how to do that, and implement it for ffmpeg, to serve as an example for other non-standard ports. The other option is to try to switch ffmpeg to vcpkg_configure_make, but that might be a much bigger undertaking, and I don't know how easy that is or even if it is possible with ffmpeg's configure script.

Pending all of this, the PR as it currently stands seems like an acceptable temporary solution for macosx deployment, at least to me, until a better solution can be implemented.

[omartijn]

Rebased on current master and solved conflicts

[omartijn]

Another round of conflict resolution.

[omartijn]

Is there anything I can do to facilitate this getting merged? I can keep resolving merge conflicts, of course, but is anybody still looking at this PR?

[PhoebeHui]

@omartijn, sorry for the delay, I will ask other team member to review this PR, please resolve the conflicts.

[PhoebeHui]

Limit knowledge to the evaluation, needs other team member to take a look again.

[strega-nil-ms]

This should probably be run after vcpkg_cmake_get_vars, and use the result from there.

[BillyONeal]

This should probably be run after vcpkg_cmake_get_vars, and use the result from there.

Please look at feeding ffmpeg the right flags with this. https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/ports/vcpkg-cmake/vcpkg_cmake_get_vars.md

(Note that we discourage use in portfiles in that documentation because most modifications to cmake vars should happen in the build system rather than the portfile, but ffmpeg has a custom build system :) )

[dg0yt]

Somewhat related (compiler, not flags): #20434

[github-actions]

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated

git add -u && git commit
git checkout 8c092e5cb6c275906758f0b5b54dde4ce6afaaa0 -- versions
./vcpkg x-add-version --all

Diff

diff --git a/versions/f-/ffmpeg.json b/versions/f-/ffmpeg.json
index e5434c5..35a4664 100644
--- a/versions/f-/ffmpeg.json
+++ b/versions/f-/ffmpeg.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "adbff70b914d111ca076f7d41421bc8c78436fde",
+      "git-tree": "24cb6ddcd146248cb3758ec1aa4c038b28d4c0c5",
       "version": "4.4",
       "port-version": 15
     },

[BillyONeal]

The macos fleet is currently broken because a SAS token expired, fix is in #20512

[strega-nil-ms]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[omartijn]

@BillyONeal Should I rebase to get the latest fixes for the azure pipeline?

[dg0yt]

Should I rebase to get the latest fixes for the azure pipeline?

@omartijn Rebasing wouldn't gain you anything wrt to the osx CI issues. The artifact cache for osx was empty when /azp run was issued, and there are lots of distant ports missing. Related: microsoft/vcpkg-tool#210

[BillyONeal]

@BillyONeal Should I rebase to get the latest fixes for the azure pipeline?

Shouldn't be needed as this build was successful. The merge to get osx fixes caused PRs to fail.

@omartijn Rebasing wouldn't gain you anything wrt to the osx CI issues. The artifact cache for osx was empty when /azp run was issued, and there are lots of distant ports missing. Related: microsoft/vcpkg-tool#210

In this case the binary cache got broken which reset all the things :/

[strega-nil-ms]

Thanks @omartijn :)