[github_actions] improve port compatibility

[cenit]

Use a parameter to avoid CMake looking for system frameworks on macOS before vcpkg-provided libs.
Reverts #22141 which is unnecessary (if this PR works and is considered effective)

[github-actions]

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

• ports/libpng/vcpkg.json
• ports/tiff/vcpkg.json

Valid values for the license field can be found in the documentation

[Neumann-A]

did it find nothing or did it find the wrong file/path?

CMake just does:

find_path(TIFF_INCLUDE_DIR tiff.h)
find_path(PNG_PNG_INCLUDE_DIR png.h PATH_SUFFIXES include/libpng)

[cenit]

did it find nothing or did it find the wrong file/path?

CMake just does:

find_path(TIFF_INCLUDE_DIR tiff.h)
find_path(PNG_PNG_INCLUDE_DIR png.h PATH_SUFFIXES include/libpng)

it finds wrong headers already installed in system folders (Mono), and on top of that they are incompatible with the ones provided by vcpkg (API breaking changes)

[Neumann-A]

it finds wrong headers already installed in system folders (Mono), and on top of that they are incompatible with the ones provided by vcpkg (API breaking changes)

That doesn't make sense. System paths should be searched after cmake paths.

according to https://cmake.org/cmake/help/latest/command/find_path.html vcpkg paths should be found under rule 2. The system stuff is rule 5/6

[cenit]

yes, it doesn't make sense, i agree. But you can try on a osx github action runner and see by yourself.

[cenit]

it's very strange also because the find_library calls are ok and find vcpkg-built libraries, only the find_path calls are broken and find headers in system folder...

[cenit]

adding to the problems, i cannot just remove Mono at the beginning, because I need it to run nuget and setup vcpkg artifact storage 😄

[Neumann-A]

yes, it doesn't make sense, i agree. But you can try on a osx github action runner and see by yourself.
it's very strange also because the find_library calls are ok and find vcpkg-built libraries, only the find_path calls are broken and find headers in system folder...

I feel like strange is not the right argumentation to merge this PR. Maybe use a overlay until you (or someone) figured out what is really going on. In general, I would expect osx to behave similar so linux so only seeing it on osx is kind of strange too.

it's very strange also because the find_library calls are ok

since vcpkg.cmake sets

function(z_vcpkg_add_vcpkg_to_cmake_path list suffix)
    set(vcpkg_paths
        "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}${suffix}"
        "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/debug${suffix}"
    )
    if(NOT DEFINED CMAKE_BUILD_TYPE OR CMAKE_BUILD_TYPE MATCHES "^[Dd][Ee][Bb][Uu][Gg]$")
        list(REVERSE vcpkg_paths) # Debug build: Put Debug paths before Release paths.
    endif()
    if(VCPKG_PREFER_SYSTEM_LIBS)
        list(APPEND "${list}" "${vcpkg_paths}")
    else()
        list(INSERT "${list}" 0 "${vcpkg_paths}") # CMake 3.15 is required for list(PREPEND ...).
    endif()
    set("${list}" "${${list}}" PARENT_SCOPE)
endfunction()
z_vcpkg_add_vcpkg_to_cmake_path(CMAKE_PREFIX_PATH "")
z_vcpkg_add_vcpkg_to_cmake_path(CMAKE_LIBRARY_PATH "/lib/manual-link")
z_vcpkg_add_vcpkg_to_cmake_path(CMAKE_FIND_ROOT_PATH "")

it could be that something nuked CMAKE_PREFIX_PATH and kept CMAKE_LIBRARY_PATH intact

[dg0yt]

GDAL added this:
OSGeo/gdal@01968e2

[cenit]

yes, it doesn't make sense, i agree. But you can try on a osx github action runner and see by yourself.

it's very strange also because the find_library calls are ok and find vcpkg-built libraries, only the find_path calls are broken and find headers in system folder...

I feel like strange is not the right argumentation to merge this PR. Maybe use a overlay until you (or someone) figured out what is really going on. In general, I would expect osx to behave similar so linux so only seeing it on osx is kind of strange too.

it's very strange also because the find_library calls are ok

since vcpkg.cmake sets

function(z_vcpkg_add_vcpkg_to_cmake_path list suffix)

    set(vcpkg_paths

        "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}${suffix}"

        "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/debug${suffix}"

    )

    if(NOT DEFINED CMAKE_BUILD_TYPE OR CMAKE_BUILD_TYPE MATCHES "^[Dd][Ee][Bb][Uu][Gg]$")

        list(REVERSE vcpkg_paths) # Debug build: Put Debug paths before Release paths.

    endif()

    if(VCPKG_PREFER_SYSTEM_LIBS)

        list(APPEND "${list}" "${vcpkg_paths}")

    else()

        list(INSERT "${list}" 0 "${vcpkg_paths}") # CMake 3.15 is required for list(PREPEND ...).

    endif()

    set("${list}" "${${list}}" PARENT_SCOPE)

endfunction()

z_vcpkg_add_vcpkg_to_cmake_path(CMAKE_PREFIX_PATH "")

z_vcpkg_add_vcpkg_to_cmake_path(CMAKE_LIBRARY_PATH "/lib/manual-link")

z_vcpkg_add_vcpkg_to_cmake_path(CMAKE_FIND_ROOT_PATH "")

it could be that something nuked CMAKE_PREFIX_PATH and kept CMAKE_LIBRARY_PATH intact

i feel confident that under similar setup also windows or linux would show same problem

without full investigation, i'd say that the problem lies in the search order defined inside the module: for example, png.h is under include/libpng16 for us, but the module first searches for png.h in include/libpng, which might roam in system folder before trying with different paths in following lines.
For this reason, it is very difficult to avoid doing this wrapping, which is not dangerous at all, it just makes sure we are choosing vcpkg provided headers.
I am not against this PR in principle, in fact i opened it because i think it's useful and it fixes an open problem for many people, which might not end with these ports. I agree with you that it might be worth a deeper investigation, but why not after this remediation is in place?

[Neumann-A]

I agree with you that it might be worth a deeper investigation, but why not after this remediation is in place?

Because @dg0yt already found the real reason. vcpkg.cmake seems to need to set CMAKE_FIND_APPBUNDLE and CMAKE_FIND_FRAMEWORK to LAST instead. In the meantime you can manually pass it to the build.

but the module first searches for png.h in include/libpng

no that is not how it works.
PATHS: Specify directories to search in addition to the default locations.
in addition not before default locations and vcpkg has png.h in /include so that should be found.

[dg0yt]

i feel confident that under similar setup also windows or linux would show same problem

I disagree. It is about macOS frameworks. See the the link in my other comment.

[github-actions]

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

• ports/giflib/vcpkg.json

Valid values for the license field can be found in the documentation

[cenit]

@dg0yt @Neumann-A thanks for the very constructive review
Is it better now?
On my ends, it looks like it's working. Another confirmation would be very welcome, I am on so many different topics right now that i might be missing something.

[Neumann-A]

cc @strega-nil-ms since she has more knowledge of osx?

[BillyONeal]

This appears to be in keeping with what vcpkg wants to do (have our bits be found first) based on my reading of the documentation for CMAKE_FIND_FRAMEWORK.

We may want to kick off a full rebuild since this touches buildsystems/vcpkg.cmake and we found out recently that checking that is probably warranted :)

[BillyONeal]

It's been a while since I've had to trigger a full rebuild, hope this is correct:

https://dev.azure.com/vcpkg/public/_build/results?buildId=68331&view=results

Edit: Well... the dreaded msiexec error has appeared

What you did is how I did it before but @ras0219 pointed out that you can queue the build against refs/pull/#/head by typing in the box rather than using the dropdown:

[cenit]

x64-osx, the only one that would have been touched by this PR, is green.
The failures in the other triplets might be baseline issues?

[cenit]

there is a warning when using this PR:

CMake Warning (dev) at F:/vcpkg/scripts/buildsystems/vcpkg.cmake:408 (set):
  Cannot set "CMAKE_FIND_FRAMEWORK": current scope has no parent.
CMake Warning (dev) at F:/vcpkg/scripts/buildsystems/vcpkg.cmake:409 (set):
  Cannot set "CMAKE_FIND_APPBUNDLE": current scope has no parent.

[JackBoosY]

@cenit Maybe related to the cmake update?

[github-actions]

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

• ports/giflib/vcpkg.json

Valid values for the license field can be found in the documentation

[JackBoosY]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[github-actions]

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

• ports/giflib/vcpkg.json

Valid values for the license field can be found in the documentation

[github-actions]

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

• ports/giflib/vcpkg.json

Valid values for the license field can be found in the documentation

[cenit]

anything more requested here? thie related issue is really blocking here for us, I still hope for a fix (this PR or anything else that might help)

[Neumann-A]

thie related issue is really blocking here for us, I still hope for a fix

It probably will get fixed but it shouldn't be blocking you. As I said you can simply pass the same parameter directly to cmake in the meantime.

[cenit]

it would require a tool chain chain loaded and more customizations for github and osx than what is reasonable for a uniform script. Passing the variable to cmake in the downstream project is not propagated upstream in the vcpkg dependencies, if this is what you mean (i think not because you know cmake well)

[cenit]

many thanks for merging ❤️

[fwcd]

Sorry for necroing this thread... deprioritizing macOS frameworks has some possibly unintended consequences when it comes to system frameworks. As a specific example, if the user has XQuartz (a popular X11 server for macOS) installed, CMake will start linking its OpenGL implementation at /usr/X11R6/lib/libGL.dylib rather than the standard system-provided OpenGL.framework (which is rarely the right thing to do).

We ran into this in Mixxx (mixxxdj/mixxx#13080), but it turns out to affect other OpenGL-based apps, causing downstream build breakages in some vcpkg ports that depend on Qt, see #38141

[cenit]

is it possible to work on the OpenGL port to promote macOS system framework in that case?