Skip to content
This repository has been archived by the owner on Dec 18, 2021. It is now read-only.

Enhance CMake files #72

Merged
merged 2 commits into from
Jul 28, 2021
Merged

Enhance CMake files #72

merged 2 commits into from
Jul 28, 2021

Conversation

kovdan01
Copy link
Contributor

  • Add install rule for sqlpp-mysql cmake package that provides sqlpp11::mysql exported target
  • Use canonical cmake way of handling dependencies via find_package instead of custom DATE_INCLUDE_DIR and SQLPP11_INCLUDE_DIR variables.
  • Do not use legacy include_directories commands.
  • Enhance FindMySQL.cmake module:
  • Run JsonTests with MariaDB as well as with MySQL.

- Add install rule for sqlpp-mysql cmake package that provides sqlpp11::mysql exported target
- Use canonical cmake way of handling dependencies via `find_package` instead of custom `DATE_INCLUDE_DIR` and `SQLPP11_INCLUDE_DIR` variables.
- Do not use legacy `include_directories` commands.
- Enhance FindMySQL.cmake module:
  - Create exported target MySQL::MySQL.
  - Fix behavior when MariaDB files are located in mysql subdirectory instead of mariadb (see Arch Linux mariadb-libs package: https://archlinux.org/packages/extra/x86_64/mariadb-libs/)
- Run JsonTests with MariaDB as well as with MySQL.
.appveyor.yml Outdated
- cmake -DCMAKE_INSTALL_PREFIX="%cd%\..\prefix" -DBUILD_TZ_LIB=OFF ..
- cmake --build . --target install
- cd ..\..
- git clone https://github.com/kovdan01/sqlpp11.git
Copy link
Owner

Choose a reason for hiding this comment

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

I guess we'll have to wait for you PR in sqlpp11 to land?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry, forgot about that. Temporarily cloned my own repository when used "Visual Studio 2013" worker image on appveyor - it only supports CMake 3.12.2, while sqlpp11 library requires CMake 3.14 or higher. Now "Visual Studio 2019" worker image with CMake 3.20.2 is used, so there is no need in using my fork. Changed to using 0.60 tag from original rbock/sqlpp11.

.travis.yml Outdated
..
- cmake --build . --target install
- cd ../..
- git clone https://github.com/kovdan01/sqlpp11.git
Copy link
Owner

Choose a reason for hiding this comment

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

same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See answer to the previous comment

@rbock
Copy link
Owner

rbock commented Jul 28, 2021

Thanks for this very thorough change and the quick update. Looks good to me.

@rbock rbock merged commit 153d115 into rbock:master Jul 28, 2021
@kovdan01
Copy link
Contributor Author

kovdan01 commented Jul 28, 2021

@rbock You're welcome, thank you for your library!

By the way, just have realized that README.md is now a bit outdated

  • DATE_INCLUDE_DIR and SQLPP11_INCLUDE_DIR are not used anymore.
  • That is not a problem anymore - in current CI build the library links with pre-installed MySQL C API. I guess that this should be removed from readme as for now.

    Remark regarding MSVC builds on appveyor: As of now, I can neither to link against the provided mysql-connector-c library nor build it from scratch. I am currently compiling the mariadb client and link against that one. Build is fine, tests fail. Help is appreciated.

Should I create a PR with readme update, or you can do it by yourself?

And one more thing: it would be cool if you could tag a new release after these changes, so readme would correspond to the latest release. If you decide to do so – please don’t forget to change library version in top-level CMakeLists.txt

@rbock
Copy link
Owner

rbock commented Jul 29, 2021

I would appreciate an other PR and would create a release afterwards.

Regarding the release number in the CMakeLists.txt: I know I will forget this at some point. I wonder if some is someone has already written a pre push hook script to prevent this?

@kovdan01
Copy link
Contributor Author

Created PR with README.md update: #74

Regarding a pre push hook script: I'll look for a good solution for this. A possible prototype is implemented in #66, but I don't like it because CMakeLists.txt depends on git (while a popular user scenario is to download release archive instead of cloning the repo; git might be even not installed in the system, especially on Windows).

I'll let you know when I find a good (from my perspective) solution.

@kovdan01 kovdan01 mentioned this pull request Jul 29, 2021
@rbock
Copy link
Owner

rbock commented Jul 30, 2021

Right, the approach in #66 is not ideal. Thanks for looking for something better.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants