Skip to content

Conversation

@AbrilRBS
Copy link
Member

@AbrilRBS AbrilRBS commented Dec 5, 2025

Changelog: Fix: defines and frameworks now also generate CMakeConfigDeps targets.
Docs: Omit

This comes as a fix for the opengl recipe in Macos, which has this package_info():

        self.cpp_info.set_property("cmake_file_name", "opengl_system")

        self.cpp_info.bindirs = []
        self.cpp_info.includedirs = []
        self.cpp_info.libdirs = []
        if self.settings.os == "Macos":
            self.cpp_info.defines.append("GL_SILENCE_DEPRECATION=1")
            self.cpp_info.frameworks.append("OpenGL")
        elif self.settings.os == "Windows":
            self.cpp_info.system_libs = ["opengl32"]
        elif self.settings.os in ["Linux", "FreeBSD", "SunOS"]:
            pkg_config = PkgConfig(self, 'gl')
            pkg_config.fill_cpp_info(self.cpp_info, is_system=self.settings.os != "FreeBSD")

which did not create a target for opengl, and then having this as a dependency made it error out

@AbrilRBS AbrilRBS added this to the 2.24.0 milestone Dec 5, 2025
Copy link
Contributor

@franramirez688 franramirez688 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AbrilRBS The PR looks good, but I think it could be the moment to refactor that a little bit?

  • The function name _get_cmake_lib sounds confusing to me. Would it not be better to have something like _get_cmake_target?
  • The first check is getting a bit convoluted and fragile:
if info.exe or not (info.package_framework or info.frameworks or info.includedirs or info.libs
                            or info.system_libs or info.defines):

What I'm wondering, if we only return empty when if info.exe. Would it not be more robust to return an empty CMake target instead? Would it be a problem to return an empty one?

    def _get_cmake_lib(self, info, components, pkg_folder, pkg_folder_var, comp_name=None):
        if info.exe:
            return

I'm asking because if we're checking the defines field, what about cflags, cxxflags, etc.?

@memsharded
Copy link
Member

It was called get_cmake_lib because there is another method called def _get_exes(self, that takes care of creating the executable targets, while this is creating the library targets.

I don't exactly recall why the check is there to not create targets if those are empty, but this is incubating, so it is good to try to remove and leave the if info.exe: return only and see how it goes

@memsharded
Copy link
Member

@franramirez688 lets merge this as-is because the change is fully clear, and have the refactor in another PR?

@franramirez688 franramirez688 merged commit 37daadb into conan-io:develop2 Dec 8, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants