Skip to content

Initial support for exporting Config.cmake with targets #81

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 27, 2022
Merged

Initial support for exporting Config.cmake with targets #81

merged 1 commit into from
Sep 27, 2022

Conversation

@wrobelda
Copy link
Collaborator

@wrobelda wrobelda commented Sep 13, 2022 •
edited
Loading

This adds support for exporting a LibOfxConfig.cmake file, together with LibOfxConfigVersion.cmake and LibOfxTargets.cmake.

Fixes #74

@wrobelda wrobelda force-pushed the config_cmake branch 6 times, most recently from 48ff468 to 8415184 Compare September 13, 2022 21:53
@wrobelda wrobelda changed the title WIP: Initial support for exporting Config.cmake with targets Initial support for exporting Config.cmake with targets Sep 13, 2022
@wrobelda wrobelda marked this pull request as ready for review September 13, 2022 21:56
@wrobelda wrobelda force-pushed the config_cmake branch 2 times, most recently from 2740edc to a67dd62 Compare September 14, 2022 09:15
@wrobelda
Copy link
Collaborator Author

wrobelda commented Sep 14, 2022 •
edited
Loading

@thesamesam I would appreciate if you could look into this as well, alongside the maintainers.

cstim
cstim approved these changes Sep 15, 2022
View reviewed changes
Copy link
Member

@cstim cstim left a comment

Choose a reason for hiding this comment

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

Looks fine.

I wonder how this could be tested, maybe even as part of the gitlab workflow? But in any case if you have tested the result so far, I'm fine with it.

@wrobelda
Copy link
Collaborator Author

wrobelda commented Sep 15, 2022 •
edited
Loading

I wonder how this could be tested, maybe even as part of the gitlab workflow? But in any case if you have tested the result so far, I'm fine with it.

I guess you could prepare a simple CMakeLists with find_package(libofx) and a target linking to it? A simple main.cpp with a header include would also test if it actually worked. I suppose this would have to run in a container, with an OpenSP provided via system repository.

I tested the result and have an MR pending for KMyMoney that will take advantage of it: https://invent.kde.org/office/kmymoney/-/merge_requests/165

thesamesam
thesamesam approved these changes Sep 15, 2022
View reviewed changes
LibOfxConfig.cmake.in
set(CMAKE_MODULE_PATH "${${CMAKE_FIND_PACKAGE_NAME}_CMAKE_MODULE_PATH_save}")
unset("${CMAKE_FIND_PACKAGE_NAME}_CMAKE_MODULE_PATH_save")
else()
find_dependency(OpenSP)
Copy link
Contributor

@thesamesam thesamesam Sep 14, 2022

Choose a reason for hiding this comment

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

Ah, the OpenSP module landed upstream? 🥳

Copy link
Collaborator Author

@wrobelda wrobelda Sep 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed it did, scheduled for 3.25 release.

@wrobelda
Copy link
Collaborator Author

wrobelda commented Sep 18, 2022 •
edited
Loading

@cstim had to push a small fix for building statically. Namely, the install(TARGETS LibOFX) was guarded with if(BUILD_SHARED_LIBS), which was wrong.

EDIT: Also, had to fix case sensitivity issues re LibOFX<>LibOfx usage in filename prefixes, to avoid installation issues on case-sensitive filesystems.

@wrobelda wrobelda force-pushed the config_cmake branch 4 times, most recently from fd9178f to fd4bb1e Compare September 23, 2022 08:49
@wrobelda
Copy link
Collaborator Author

wrobelda commented Sep 23, 2022

Also removed LibOFX_BINDIR from Config.cmake.in: tools are irrelevant from library consumer's point of view and not enabling any of them for compilation would cause set_and_check(LibOFX_BINDIR) to fail.

@wrobelda
Copy link
Collaborator Author

wrobelda commented Sep 24, 2022 •
edited
Loading

@cstim any chance you could look into merging these and releasing a 0.10.8 over the weekend? I have a pending PR for vcpkg portfile and would love to have it merged to their master sometime soon.

cstim
cstim requested changes Sep 25, 2022
View reviewed changes
Copy link
Member

@cstim cstim left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The changing target library name in lib/CMakeLists.txt doesn't get accepted by me. The resulting library file on Linux needs to continue to be libofx.so.7.0.2 but in order to achieve this, the add_library target's name must be ofx exactly and not LibOFX.

I'll check whether I can cherry-pick any of the other changes quickly, though...

lib/CMakeLists.txt
set(ofx_HEADERS ${ofx_HEADERS} win32.hh)
endif ()

add_library(ofx ${ofx_SRCS} ${ofx_HEADERS})
Copy link
Member

@cstim cstim Sep 25, 2022

Choose a reason for hiding this comment

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

Sorry, this change on Linux ends up in a file libLibOFX.so.7. The target name used in add_library shouldn't include the "lib..." prefix.

Copy link
Collaborator Author

@wrobelda wrobelda Sep 25, 2022

Choose a reason for hiding this comment

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

Fair enough. I'll fix it and let you know.

Copy link
Collaborator Author

@wrobelda wrobelda Sep 25, 2022

Choose a reason for hiding this comment

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

Just wondering what should the target name be? LibOFX::ofx, libofx::ofx or ofx::ofx? I feel like the latter is too generic.

Copy link
Collaborator Author

@wrobelda wrobelda Sep 25, 2022

Choose a reason for hiding this comment

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

OK, I managed to get it to be libofx::libofx and keep the libofx library name at the same time

cstim
cstim approved these changes Sep 27, 2022
View reviewed changes
Copy link
Member

@cstim cstim left a comment

Choose a reason for hiding this comment

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

Ok, fine now. Thanks a lot!

@cstim cstim merged commit 33a4a2d into libofx:master Sep 27, 2022
@wrobelda wrobelda mentioned this pull request Sep 27, 2022
Closed
@wrobelda wrobelda mentioned this pull request Oct 10, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cstim cstim cstim approved these changes

+1 more reviewer

@thesamesam thesamesam thesamesam approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Missing LibofxConfig.cmake

3 participants

@wrobelda @cstim @thesamesam