Skip to content

make minja packagable by setting target in cmake#62

Merged
ochafik merged 3 commits intogoogle:mainfrom
jhlee525:cmake-packaging
Apr 24, 2025
Merged

make minja packagable by setting target in cmake#62
ochafik merged 3 commits intogoogle:mainfrom
jhlee525:cmake-packaging

Conversation

@jhlee525
Copy link
Contributor

Functionality of the minja library works well, and I think it's a great library. But it lacks proper CMake packaging, which makes it difficult to integrate smoothly within other CMake-based projects.

This PR introduces the following changes:

  1. Install Command
    The install command has been added so that running running make install will let you put the header files into $CMAKE_INSTALL_PREFIX/include/minja.

  2. FetchContent Integration
    A CMake target has been defined to support users who want to include minja using FetchContent. This means other C++ projects to can import minja like this:

FetchContent_Declare(minja URL "minja-url")
FetchContent_MakeAvailable(minja)
  1. Optional Python Dependency
    Currently, minja's CMake configuration requires a Python interpreter. For users who only need the header files, two options have been added: MINJA_TEST_ENABLED and MINJA_EXAMPLE_ENABLED. By disabling these options, users can skip unnecessary CMake steps and avoid the Python dependency when it is not needed.

@google-cla
Copy link

google-cla bot commented Apr 17, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jhlee525
Copy link
Contributor Author

CI fails in reading files, for example

3: C++ exception with description "Failed to open file: tests/CohereForAI-c4ai-command-r7b-12-2024-tool_use.jinja" thrown in the test body.

However, I have no idea how test can get these files in CI

@ochafik
Copy link
Contributor

ochafik commented Apr 18, 2025

Can confirm your branch runs great locally 👌. CI needs a HuggingFace key and I haven't sorted out a setup that prevents leaking it yet, sorry about this.

Looks like cmake --install build installs gtest & gmock too, wonder if there's a way to prevent this (besides disabling tests)? And maybe add a section to the top-level README.md w/ similar instructions to this PR's description?

@jhlee525
Copy link
Contributor Author

I missed testing install with test option. It turns out that googletest having INSTALL_GTEST in CMakeLists.txt which forces installing itself (when using FetchContents).

So I put a line to disable this option.

@jhlee525
Copy link
Contributor Author

Also, as well as instruction

@ochafik ochafik merged commit b152576 into google:main Apr 24, 2025
3 of 11 checks passed
@ochafik
Copy link
Contributor

ochafik commented Apr 24, 2025

Thanks a lot @jhlee525, merged!!

@jhlee525 jhlee525 deleted the cmake-packaging branch April 30, 2025 02:51
ochafik pushed a commit to ochafik/minja that referenced this pull request Nov 2, 2025
* make minja packagable by setting target in cmake

* preventing gtest install

* Add cmake instruct in README
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.

2 participants