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.

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

glog fails to resolve its gflags dependency when GFLAGS_USE_TARGET_NAMESPACE is ON #18733

Closed
bucurb opened this issue Jun 30, 2021 · 10 comments · Fixed by #18739
Closed

glog fails to resolve its gflags dependency when GFLAGS_USE_TARGET_NAMESPACE is ON #18733

bucurb opened this issue Jun 30, 2021 · 10 comments · Fixed by #18739
Assignees
Labels
category:port-bug The issue is with a library, which is something the port should already support

Comments

@bucurb
Copy link
Contributor

bucurb commented Jun 30, 2021

Describe the bug
glog fails to link gflags if used in a project that defines GFLAGS_USE_TARGET_NAMESPACE

Environment

  • OS: any
  • Compiler: any

To Reproduce
Steps to reproduce the behavior:

  1. ./vcpkg install glog --triplet=x64-windows-static
  2. The above will install both glog and its dependency gflags
  3. Create a dummy project with a CMakefile that does: set(GFLAGS_USE_TARGET_NAMESPACE ON) find_package(glog CONFIG REQUIRED) find_package(gflags CONFIG REQUIRED)
  4. Have a cmake target that target_link_libraries(blah glog::glog gflags::gflags_static)

Notice how target blah fails to link as glog depends on target gflags which is not defined due to the gflags:: namespace.

Expected behavior
glog should be able to find gflags::gflags in this case. More generally for the static build case it probably doesn't make a lot of sense to be able to mix gflags and gflags::gflags, so it should be possible to decide on either with or without namespace and have all dependent libraries respect that setting.

Failure logs
N/A

Additional context
N/A

@NancyLi1013
Copy link
Contributor

Hi @bucurb

Thanks for posting this issue.

I have some questions here. Could you please help clarify these?
Why do you need to defines GFLAGS_USE_TARGET_NAMESPACE in your project?

Where do you get GFLAGS_USE_TARGET_NAMESPACE ? I cannot find this on glog and gflags.

For the usage, it's unnecessary to add gflagsin your project, since glog has the dependency of gflags. You just use glog like this:

    find_package(glog CONFIG REQUIRED)
    target_link_libraries(main PRIVATE glog::glog)

@bucurb
Copy link
Contributor Author

bucurb commented Jul 1, 2021

Hi @NancyLi1013

You're right, it's not necessary to add gflags to my project. However, the problem is with the glog dependency. The GFLAGS_USE_TARGET_NAMESPACE variable is checked here . gflags exposes 2 flavors of targets, one with no namespace and one with the gflags:: namespace. In my project I'd like to set GFLAGS_USE_TARGET_NAMESPACE so that gflags exposes the gflags::gflags target. The problem is glog is not aware of this and continues to reference gflags as its dependency.

I'm in a project where everything is statically built to libraries, so in this case the glog -> gflags dependency is resolved when my target is linked, at which point I only have a gflags::gflags target. I think this is not a problem for dll builds, as there glog can happily resolve to gflags privately.

@NancyLi1013 NancyLi1013 added the category:port-bug The issue is with a library, which is something the port should already support label Jul 1, 2021
@NancyLi1013
Copy link
Contributor

Is your error like this?

LINK : fatal error LNK1104: cannot open file 'gflags.lib' [F:\cmaketest\glog\build\main.vcxproj]

@JackBoosY
Copy link
Contributor

JackBoosY commented Jul 1, 2021

When building dynamic, the gflags targets are gflags_shared and gflags::gflags_shared,
and when building static, its targets are gflags_static and gflags::gflags_static.

After reading the generated cmake configuration files, I feel that the contents between them are completely the same except for the namespace.
So I think your changes is not correct.

@bucurb
Copy link
Contributor Author

bucurb commented Jul 1, 2021

@NancyLi1013 yes, exactly

@bucurb
Copy link
Contributor Author

bucurb commented Jul 1, 2021

@JackBoosY so starting from my project's CMakefile:

set(GFLAGS_USE_TARGET_NAMESPACE ON)
find_package(gflags CONFIG REQUIRED) 

we end-up in gflags-config.cmake where we hit this:

if (GFLAGS_USE_TARGET_NAMESPACE)
  include ("${CMAKE_CURRENT_LIST_DIR}/gflags-targets.cmake")
  set (GFLAGS_TARGET_NAMESPACE gflags)
else ()
  include ("${CMAKE_CURRENT_LIST_DIR}/gflags-nonamespace-targets.cmake")
  set (GFLAGS_TARGET_NAMESPACE)
endif ()

gflags-targets.cmake only defines gflags::gflags_static

foreach(_expectedTarget gflags::gflags_static)

What is defining the non-namespaced gflags_static target? I didn't find anything

@JackBoosY
Copy link
Contributor

@bucurb
You can compare gflags-targets*.cmake with gflags-nonamespace-targets*.cmake. The only difference between them is whether there is a namespace. This proves that the effect of using a target that does not contain a namespace is exactly the same as a target that contains a namespace. Just because the upstream provides the namespace.

Check gflags-config.cmake carefully. In addition to including gflags-targets.cmake and gflags-nonamespace-targets.cmake, it also provides unified target gflags or gflags::gflags (according to whether external GFLAGS_USE_TARGET_NAMESPACE is defined) to avoid the inconsistency of dynamic and static target names. At the end, it also provides the macros which FindGflags.cmake provided.

In summary, the use of any target is consistent.

@bucurb
Copy link
Contributor Author

bucurb commented Jul 2, 2021

@JackBoosY you are right on both counts of course, but I never said there is a difference between the namespaced and non-namespaced targets. Yes, they're identical except for the name, as they should be. I also know about the gflags / gflags::gflags targets being added to simplify picking between static and shared targets. Again, this is not my problem.

My problem is that in a project you either get the target gflags or the target gflags::gflags, but not both in the same time - assuming you have a project-wide GFLAGS_USE_TARGET_NAMESPACE defined. Now glog has a dependency to target gflags. If your project only knows about gflags::gflags, then if you want to depend on glog your project won't compile as it will try to (statically) link against gflags. @NancyLi1013 seems to have already reproduced the linker error I was getting.

If you're saying there is a way to have both GFLAGS_USE_TARGET_NAMESPACE = ON and get past the linker error without any changes to vcpkg I'd be happy to remove my local patch and give it a try!

@JackBoosY
Copy link
Contributor

JackBoosY commented Jul 2, 2021

@bucurb Yes, the problem is GFLAGS_USE_TARGET_NAMESPACE, in addition, we should provide the file usage in ports/gflags (provide a unified target gflags::gflags or gflags according to the value of GFLAGS_USE_TARGET_NAMESPACE) and Install to share/${PORT} to notify users of usage.

In addition, we should also provide vcpkg-cmake-wrapper.cmake and install it to share/${PORT} to prevent users from modifying the macro GFLAGS_USE_TARGET_NAMESPACE.

@bucurb
Copy link
Contributor Author

bucurb commented Jul 2, 2021

@JackBoosY yup, so that's pretty much what I was trying to achieve with my PR. If you choose to install gflags[target-namespace] then you will get a vcpkg-cmake-wrapper.cmake that sets GFLAGS_USE_TARGET_NAMESPACE = ON every time you find_package(gflags). Which means that, even when vcpkg builds packages, it will see the namespaced gflags::gflags target and won't be able to simply use gflags (as glog does now). Maybe it would be more productive at this stage to just move the discussion to the PR itself so you can point out the bits that you'd like changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants