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

[fmt] conan v2 compatibility #10456

Merged
merged 26 commits into from Jul 20, 2022
Merged

[fmt] conan v2 compatibility #10456

merged 26 commits into from Jul 20, 2022

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Apr 23, 2022

Specify library name and version: fmt/all

a port from czoido@5708ca4
/cc @czoido

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

IMHO we should keep a working test_xxxxx/conanfile.py using stable generators (cmake + cmake_find_package[_multi]). Would you mind keeping that example too?

@SSE4
Copy link
Contributor Author

SSE4 commented Apr 25, 2022

IMHO we should keep a working test_xxxxx/conanfile.py using stable generators (cmake + cmake_find_package[_multi]). Would you mind keeping that example too?

these generators aren't stable, they are marked as "experimental": https://docs.conan.io/en/latest/reference/generators/cmake_find_package_multi.html

I would keep only examples using current best practices going forward (CMakeDeps/CMakeToolchain).

UPD: last year I've tried to promote these generators conan-io/docs#2269, but seems like there was not enough interest to make them officially recommended. anyway, given now we have at least 7 different CMake generators available, it's easy for newcomers to get confused and completely lost. I think going forward, it only makes sense to promote CMakeDeps/CMakeToolchain as recommended, as others will just disappear soon.

@conan-center-bot

This comment has been minimized.

@SSE4 SSE4 closed this May 27, 2022
@SSE4 SSE4 reopened this May 27, 2022
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@ericLemanissierBot
Copy link

I detected other pull requests that are modifying fmt/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@jgsogo
Copy link
Contributor

jgsogo commented Jul 13, 2022

@SSE4 , please, fix conflicts... let's see if this one works with v2

czoido and others added 4 commits July 13, 2022 16:21
* add fmt for 2.0

* safe rm

* remove names

* add definition

* fix

* names back

* remove export_sources

* add exports_sources
Signed-off-by: SSE4 <tomskside@gmail.com>
Signed-off-by: SSE4 <tomskside@gmail.com>
Signed-off-by: SSE4 <tomskside@gmail.com>
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@SSE4 SSE4 requested a review from czoido July 16, 2022 12:08
@conan-center-bot

This comment has been minimized.

@czoido
Copy link
Contributor

czoido commented Jul 18, 2022

@SSE4 the package_info I proposed was failing because the components in Conan 1.50 don't have default libdirs, includedirs, etc. and the hook 54 failed. It would be fixed with this updated package_info:

    def package_info(self):
        self.cpp_info.names["cmake_find_package"] = "fmt"
        self.cpp_info.names["cmake_find_package_multi"] = "fmt"
        self.cpp_info.names["pkg_config"] = "fmt"

        target = "fmt-header-only" if self.options.header_only else "fmt"
        self.cpp_info.set_property("cmake_target_name", "fmt::{}".format(target))

        # TODO: back to global scope in conan v2 once cmake_find_package* generators removed
        self.cpp_info.components["_fmt"].includedirs.extend(["include"])
        if self.options.header_only:
            self.cpp_info.components["_fmt"].defines.append("FMT_HEADER_ONLY=1")
            if self.options.with_fmt_alias:
                self.cpp_info.components["_fmt"].defines.append("FMT_STRING_ALIAS=1")
        else:
            postfix = "d" if self.settings.build_type == "Debug" else ""
            libname = "fmt" + postfix
            self.cpp_info.components["_fmt"].libs = [libname]
            # FIXME: remove when Conan 1.50 is used in c3i and update the Conan required version
            # from that version components don't have empty libdirs by default
            self.cpp_info.components["_fmt"].libdirs.extend(["lib"])
            if self.options.with_fmt_alias:
                self.cpp_info.components["_fmt"].defines.append("FMT_STRING_ALIAS=1")
            if self.options.shared:
                self.cpp_info.components["_fmt"].defines.append("FMT_SHARED")

        # TODO: to remove in conan v2 once cmake_find_package* generators removed
        self.cpp_info.components["_fmt"].names["cmake_find_package"] = target
        self.cpp_info.components["_fmt"].names["cmake_find_package_multi"] = target
        self.cpp_info.components["_fmt"].set_property("cmake_target_name", "fmt::{}".format(target))

This is more aligned with what we do in other recipes, but I would be ok with merging the current package_info as is but adding a FIXME or some comments saying that the reason of having a component there is just to create a new target name for header-only.

@SSE4 SSE4 closed this Jul 18, 2022
@SSE4 SSE4 reopened this Jul 18, 2022
@conan-center-bot

This comment has been minimized.

@SSE4
Copy link
Contributor Author

SSE4 commented Jul 18, 2022

@czoido seems like it still fails for Windows/shared configs. might it be due to missing default bindirs on components?

@czoido
Copy link
Contributor

czoido commented Jul 18, 2022

@czoido seems like it still fails for Windows/shared configs. might it be due to missing default bindirs on components?

Yes, I think that could be the reason....

@conan-center-bot

This comment has been minimized.

@github-actions
Copy link
Contributor

Hooks produced the following warnings for commit 48eecaf
fmt/5.3.0
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library './lib/libfmt.so' links to system library 'm' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library './lib/libfmtd.so' links to system library 'm' but it is not in cpp_info.system_libs.
fmt/9.0.0
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library './lib/libfmtd.so' links to system library 'm' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library './lib/libfmt.so' links to system library 'm' but it is not in cpp_info.system_libs.

Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

Super excited seeing this working!!!

recipes/fmt/all/conanfile.py Outdated Show resolved Hide resolved
"FMT_LIB_DIR": "lib"
}
if self._has_with_os_api_option:
cache_entries["FMT_OS"] = self.options.with_os_api
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious about the other variable mechanics https://docs.conan.io/en/latest/reference/conanfile/tools/cmake/cmaketoolchain.html#variables

If the toolchain sets the value before the options are declared then they do nothing according to https://cmake.org/cmake/help/latest/command/option.html

If this is set to use 1.49 then I think we could get away for using just tc.variables however I agree the cache ones are better UX this seems to be working for my project

SSE4 added 2 commits July 19, 2022 13:10
Signed-off-by: SSE4 <tomskside@gmail.com>
Signed-off-by: SSE4 <tomskside@gmail.com>
@conan-center-bot
Copy link
Collaborator

All green in build 30 (32eced70afe823b2204860fd4f4ed25ad1d5a90f):

  • fmt/5.3.0@:
    All packages built successfully! (All logs)

  • fmt/7.1.3@:
    All packages built successfully! (All logs)

  • fmt/9.0.0@:
    All packages built successfully! (All logs)

  • fmt/8.0.1@:
    All packages built successfully! (All logs)

  • fmt/6.2.1@:
    All packages built successfully! (All logs)

  • fmt/8.1.1@:
    All packages built successfully! (All logs)

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

This is a good starting point for a working v2 recipe, some details can be polished later.

@conan-center-bot conan-center-bot merged commit de720f0 into conan-io:master Jul 20, 2022
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.

None yet

10 participants