Skip to content

Add CMake Config-file package #260

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

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

kevinAlbs
Copy link

@kevinAlbs kevinAlbs commented Jan 15, 2024

Summary

This PR resolves #250 by creating CMake Config-file Package.

Motivation

https://cmake.org/cmake/help/latest/manual/cmake-packages.7.html#config-file-packages notes:

Config-file packages are provided by upstream vendors as part of development packages, that is, they belong with the header files and any other files provided to assist downstreams in using the package

Including a CMake Config-file Package enables CMake consumers to use find_package to find and use utf8proc:

add_executable (app app.c)
find_package (utf8proc 2.9.0 REQUIRED)
target_link_libraries (app PRIVATE utf8proc::utf8proc)

The absence of a CMake Config-file Package may add difficulty using utf8proc in a CMake project. For example: the MongoDB C driver includes FindUtf8Proc.cmake using pkg-config. However, using pkg-config is not suitable for consumers building with MSVC (the -l and -L flags are unrecognized).

Use of relative paths in 422de38 is intended to make the installed package relocatable.

@kevinAlbs kevinAlbs changed the title Add cmake config file package Add CMake Config-file package Jan 15, 2024
@kevinAlbs kevinAlbs marked this pull request as ready for review January 15, 2024 15:58
kevinAlbs and others added 4 commits June 7, 2024 12:48
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
utf8proc appears to use SemVer. Do not consider different major versions compatible.
@kevinAlbs
Copy link
Author

Thank you for the review @kou. Feedback has been applied.

@kevinAlbs kevinAlbs requested a review from kou June 7, 2024 17:00
Copy link

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

(But I'm not a maintainer of this project. I just an user of this project.)

@kevinAlbs
Copy link
Author

@stevengj may I request merging this PR? This may help using utf8proc with CMake.

@kou
Copy link

kou commented May 16, 2025

@stevengj @eschnett Could you take a look at this?

@eschnett
Copy link
Collaborator

This is a nice modernization of the cmake file. However, I see that these changes are not tested in CI. Did you test them? Do you have a package that uses utf8proc via cmake, using the mechanisms you describe?

If a package is undergoing regular development, then changes to the build system are automatically tested. utf8proc is currently in a rather quiet stage, and as such, all changes need to be well tested – otherwise an error might creep in, and people would only notice a year from now when the next feature change is made, and downstream package are trying to use cmake to build utf8proc.

I know that it is very uncommon to have CI test the packaging mechanism. I argue it should in general, but I'm not going to insist since this would be a lot of work. I still would like you to confirm that you tested all the features you're introducing. (Kudos for updating the documentation explaining how to use cmake in a modern way.)

@kou
Copy link

kou commented May 16, 2025

We can add a CI job for the CMake package by just creating a small CMake project something like:

/* app.c */
#include <stdio.h>
#include <utf8proc.h>

int
main(void)
{
  printf("%s\n", utf8proc_version());
  return 0;
}
# CMakeLists.txt
cmake_minimum_required(VERSION 3.16)
project(utf8proc-test)
find_package(utf8proc REQUIRED)
add_executable(app app.c)
target_link_libraries(app utf8proc::utf8proc)
$ cmake -S . -B build -DCMAKE_PREFIX_PATH=${UTF8PROC_PREFIX}
$ make -C build

@kevinAlbs Can you add a CI job for this CMake package? (Or I can work work on it.)

FYI: I'm using utf8proc in https://github.com/apache/arrow . But we use our Findutf8proc.cmake https://github.com/apache/arrow/blob/main/cpp/cmake_modules/Findutf8proc.cmake not this CMake package for now.

@kevinAlbs
Copy link
Author

@kevinAlbs Can you add a CI job for this CMake package?

Done. 8ed21d9 adds a test to GitHub actions for consuming the package with CMake. Thank you for the starter code.

@kevinAlbs kevinAlbs requested review from kou and stevengj May 17, 2025 18:46
- name: Test Consuming (Windows)
if: ${{ matrix.os == 'windows-latest' }}
run: |
cmake --install build --prefix tmp/install --config Debug
Copy link

Choose a reason for hiding this comment

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

How about adding -DCMAKE_INSTALL_PREFIX=tmp/install to cmake -S . ... in the name: Build step instead of using --prefix tmp/install here?

.gitignore Outdated
@@ -28,6 +28,7 @@
/test/case
/test/iscase
/test/custom
/test/app/build
Copy link

Choose a reason for hiding this comment

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

I'm not sure we need this.

@eschnett Do you want to add this? If we want to use cmake -S test/app -B test/app/build ... for local development, this will be helpful. If we want to use cmake -S test/app -B ../test-app-build ... or something for local development, this is needless.

FYI: Our README has cmake -S . -B build but we don't have /build/ in this list.
https://github.com/JuliaStrings/utf8proc?tab=readme-ov-file#quick-start

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would not add this. I would only add directories or files that are generated by the build system. The path where cmake builds is under user control and I frequently use different paths than those suggested in a packages's documentation. I have, however, no strong opinion on this.

Copy link
Author

Choose a reason for hiding this comment

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

Removed with 60205e4.

Copy link

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

Use capital `$Env` instead of `$env` to match conventions. 
Prefix `PATH` to avoid referring unintentional binaries.
Prefer `\` over `/` to match conventions.

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
kevinAlbs and others added 5 commits May 19, 2025 20:03
To allows future Windows additions to the test matrix.

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Remove unneeded export.
Prefix `PATH` to avoid referring unintentional binaries.

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
@kou
Copy link

kou commented May 22, 2025

@stevengj @eschnett Could you approve CI jobs and review this? If there is no more concerns, could you merge this?

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.

CMake export configuration missing
4 participants