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

CMake: add cmake installation; add SIMPLEINI_USE_SYSTEM_GTEST option #72

Merged
merged 3 commits into from
Dec 29, 2023

Conversation

topazus
Copy link
Contributor

@topazus topazus commented Dec 24, 2023

The PR does the folllowing things, which will help for packaging in Linux when using CMake:

  • Generate the CMake config, version and target files, and install them
  • Installation of header file
  • Add SIMPLEINI_USE_SYSTEM_GTEST option to choose to use system gtest dependency or not.

@brofield
Copy link
Owner

I'm happy that SimpleIni is getting some love from the CMake users, but it concerns me that none of these contributions are tested in any way by the build or test harness. What do you think about contributing a change so that build and execute of the test harness uses CMake?

@dacap
Copy link
Contributor

dacap commented Dec 28, 2023

Just making some comments in this PR as I was asked for:

  1. This PR has two different changes (probably would be better to have two commits)
  2. I agree about SIMPLEINI_USE_SYSTEM_GTEST, it's a good option for Linux platforms
  3. I'm not used to all the GNUInstallDirs options, so I cannot make a valid point about them but it looks like something needed to do a "make install"

It would be nice to include some alternative GitHub workflow to test the cmake toolchain.

@topazus
Copy link
Contributor Author

topazus commented Dec 28, 2023

  1. This PR has two different changes (probably would be better to have two commits)

Thanks for your advice, I will split it into two commits.

  • I agree about SIMPLEINI_USE_SYSTEM_GTEST, it's a good option for Linux platforms
  • I'm not used to all the GNUInstallDirs options, so I cannot make a valid point about them but it looks like something needed to do a "make install"

Yes, use system gtest and use GNUInstallDirs will facilitate to help with Linux packaging. Btw, I am trying to make simpleini package into Fedora official repository. By applying this patch, it seems fine when building RPM package. see build log: https://kojipkgs.fedoraproject.org//work/tasks/1336/110961336/build.log

@brofield
Copy link
Owner

Thank you for your contribution.

@brofield brofield merged commit 6f9a279 into brofield:master Dec 29, 2023
2 checks passed
@brofield
Copy link
Owner

brofield commented Dec 2, 2024

Looking more into the cmake build harness, and I'm wondering why was the SIMPLEINI_USE_SYSTEM_GTEST option necessary? Wouldn't adding the package to the FetchContent_Declare statement be best, see Using Dependencies Guide? i.e. use the installed package and if not available then build it outselves:

include(FetchContent)
FetchContent_Declare(
	googletest
	DOWNLOAD_EXTRACT_TIMESTAMP ON
	URL https://github.com/google/googletest/archive/refs/tags/v1.14.0.zip
	URL_HASH SHA1=0ac421f2ec11af38b0fff0f1992184032731a8bc
	FIND_PACKAGE_ARGS NAMES GTest
	)
FetchContent_MakeAvailable(googletest)

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