cc @cenit @dg0yt

Thanks a lot @LilyWangLL / @JackBoosY , these changes look good to me (bug reporter). I just hope removing PARENT_SCOPE isn't going to break someone else's workflow, I'm most likely not familiar with all possible use cases.

I just hope removing PARENT_SCOPE isn't going to break someone else's workflow, I'm most likely not familiar with all possible use cases.

removing PARENT_SCOPE is correct. find_package is like a glorified include statement. If it breaks someone the person who is getting broke is simply wrong.

@Neumann-A Thanks for the clarification.

/azp run

@LilyWangLL merge with master again?

You should revert cf01dcf

PARENT_SCOPE in the wrapper is wrong!

You should revert cf01dcf

PARENT_SCOPE in the wrapper is wrong!

But if doesn't use PARENT_SCOPE, other ports will be affected. Like openmvs opencv4.
cc @cenit @dg0yt
PARENT_SCOPE and these codes added by commit 4015784.

set(HARFBUZZ_LIBRARY harfbuzz::harfbuzz PARENT_SCOPE)
set(HARFBUZZ_LIBRARIES harfbuzz::harfbuzz PARENT_SCOPE)
set(HARFBUZZ_INCLUDE_DIR "${_INSTALL_DIR}/include/harfbuzz" PARENT_SCOPE)
set(HARFBUZZ_INCLUDE_DIRS "${_INSTALL_DIR}/include/harfbuzz" PARENT_SCOPE)

if(HARFBUZZ_LIBRARY_RELEASE)
  set(HARFBUZZ_FOUND TRUE PARENT_SCOPE)
else()
  set(HARFBUZZ_FOUND FALSE PARENT_SCOPE)
endif()

But if doesn't use PARENT_SCOPE, other ports will be affected. Like openmvs opencv4.

Then those ports are doing it wrong and need fixing

But if doesn't use PARENT_SCOPE, other ports will be affected. Like openmvs opencv4.

Then those ports are doing it wrong and need fixing

If the config file only provides targets, we should probably add these macros to the wrapper in order to adapt to the MODULE mode.
Clearly, however, the hierarchy creates a problem here.

If the config file only provides targets, we should probably add these macros to the wrapper in order to adapt to the MODULE mode.

Wait are some of those variables CACHE variables in MODULE mode ? If yes set them as CACHE variables !

Thanks everyone for working on this!

I'd just like to remind that this PR is supposed to fix the following bug: #23694

The bug is that calling find_package(harfbuzz) in any CMakeLists produces the following warnings:

Cannot set "HARFBUZZ_LIBRARY": current scope has no parent.
Cannot set "HARFBUZZ_LIBRARIES": current scope has no parent.
Cannot set "HARFBUZZ_INCLUDE_DIR": current scope has no parent.
Cannot set "HARFBUZZ_INCLUDE_DIRS": current scope has no parent.
Cannot set "HARFBUZZ_FOUND": current scope has no parent.

because of all the lines in harfbuzzConfig.cmake.in which set a variable using PARENT_SCOPE, where in fact there is no parent scope. As of now, the PR isn't modifying these lines. It is just adding other lines, so the warning will remain and the bug will not be fixed.

I see a couple of options:

1. Remove all PARENT_SCOPE in harfbuzzConfig.cmake.in (original commit of this PR), and fix opencv and other clients so that they do not need the variables to be set at parent scope. This seems like the ideal fix but maybe not easy, I don't know.
2. As a workaround, in harfbuzzConfig.cmake.in, test whether there is a parent scope, and use PARENT_SCOPE or not accordingly. This seems fragile... what of use cases where there do exist a parent scope, but the client still expects the variables to be set at current scope? A possibility might be that when there is a parent scope, then the variables are set both as parent scope and current scope. And when there is no parent scope, there are set at current scope only.
3. As a workaround, in harfbuzzConfig.cmake.in, set the variables as cache variables instead of normal variables. This seems like the easiest solution to implement, but perhaps a bit too invasive and not ideal.

Thanks!

If the config file only provides targets, we should probably add these macros to the wrapper in order to adapt to the MODULE mode. Clearly, however, the hierarchy creates a problem here.

There is no official Find module for Harfbuzz, so there cannot be a wrapper.

And going one step further, if there is no official Find module and upstream doesn't provide these variables in exported config, then the variables shouldn't be patched in by vcpkg IMHO. This is the wrong direction of dependencies, and it is inviting user dependencies on non-standard variables.
Instead, set variables as needed in consuming ports.
And if you need an ABC_FOUND for abc config, there find_package(ABC NAMES abc).

Since implementing the "most correct" solution (fixing ports depending on Harfbuzz) seems to not get any traction, I would suggest in the meantime to implement the following workaround: testing if the current scope has a parent and applying PARENT_SCOPE or not accordingly. It would look like this:

# Set Harbuzz variables, either at the current scope or parent scope depending on the existence of
# a parent scope. Ideally, we should never set the variable at parent scope, but it is a workaround
# for some ports that expect this behavior. See https://github.com/microsoft/vcpkg/issues/23694
get_directory_property(hasParent PARENT_DIRECTORY)
if(hasParent)
    set(HARFBUZZ_LIBRARIES harfbuzz::harfbuzz PARENT_SCOPE)
    set(HARFBUZZ_INCLUDE_DIR "${_INSTALL_DIR}/include/harfbuzz" PARENT_SCOPE)
    set(HARFBUZZ_INCLUDE_DIRS "${_INSTALL_DIR}/include/harfbuzz" PARENT_SCOPE)
    if(HARFBUZZ_LIBRARY_RELEASE)
        set(HARFBUZZ_FOUND TRUE PARENT_SCOPE)
    else()
        set(HARFBUZZ_FOUND FALSE PARENT_SCOPE)
    endif()
else()
    set(HARFBUZZ_LIBRARIES harfbuzz::harfbuzz)
    set(HARFBUZZ_INCLUDE_DIR "${_INSTALL_DIR}/include/harfbuzz")
    set(HARFBUZZ_INCLUDE_DIRS "${_INSTALL_DIR}/include/harfbuzz")
    if(HARFBUZZ_LIBRARY_RELEASE)
        set(HARFBUZZ_FOUND TRUE)
    else()
        set(HARFBUZZ_FOUND FALSE)
    endif()
endif()

Should I make a PR with such change? Or maybe @LilyWangLL you could integrate this change to this PR so we can see if the checks pass with this approach?

Thanks!

Any updates on this?

Any updates on this?

Sorry I missed the previous comment, I will test this soon. Thanks very much for your suggestion~

@LilyWangLL Perfect, thanks a lot!

Closed as duplicate to #25092