Add `USE_SYSTEM_DATE` cmake option #372

kovdan01 · 2021-07-27T09:50:25Z

A user might already have a system/self-built version of a dependency and want to use it. For example, such version might be newer and have some fixes, including security ones. For such users, USE_SYSTEM_DATE option (for Howard Hinnant's date library) will be helpful.

rbock · 2021-07-28T05:25:09Z

Being a CMake illiterate, I'll say this sounds convincing, but I'll give @Leon0402 a few days to chime in as he introduced FetchContent here..

I kinda hope that the CMake files will converge to an accepted solution at some point :-)

Leon0402 · 2021-07-28T06:26:26Z

* The machine where the library is being built **must** be connected to the Internet in order to download dependencies. That is not always possible - sometimes you just have the sources and want to build them without any additional network activity.

So you mean you downloaded the library on a machine with internet and then copied it to another machine, which has no internet to build it there?
You can just download the other dependencies as well and set FETCHCONTENT_SOURCE_DIR_ to the local projects. I think this is acceptable for such a special case, you probably have done that anyway with find_package?
(In case the lib is already installed see below)

Using FetchContent_MakeAvailable limits cmake minimum version to 3.14, which is only released on 2019-10-02. Without that, cmake_minimum_required can be set to 3.12.

That is correct. We agreed on that to be acceptable for this library and it's connectors, when I introduced the change. One reason for this was that the newest ubuntu LTS ships with that version, which we took here as a reference.
That said, we could always use the longer replacement of FetchContent_MakeAvailable if this is a serious issue.

* A user might already have a system/self-built version of a dependency and want to use it. For example, such version might be newer and have some fixes, including security ones.

There are a few scenarios here:

You built sqlpp11 yourself as well -> Then just either change the version to the one you need or use find_package. If you actively decide to ignore the version pin we made here, then it's on your own risk obviously. (PRs for version bumps are welcome btw.)
This is something find_package wouldn't solve any better. You can specify a version with fetch_Content and you can with find_package or you can leave it away. Say we would pin find_package to same version then you would still need to manually adjust that. And I think it's totally fine that you adjust it to your needs, if you know what you're doing
You use sqlpp11 with FetchContent directly. Even easier than that. Just make a FetchContent_Declare to Date before you Make_Available sqlpp11 with the version you need and it will work flawlessly. CMake allows you to override these things. One of the reasons I would recommend you use FetchContent as well.

* Directly downloading date library by Howard Hinnant instead of using it in a canonical way (using `find_package(date REQUIRED)`, assuming the library is in the system-wide prefix or its prefix is listed in `CMAKE_PREFIX_PATH`) is pretty strange - a sqlpp11's user will have to have this library installed among with its cmake packages anyway (see [this](https://github.com/rbock/sqlpp11/blob/develop/cmake/Sqlpp11Config.cmake#L29)).

What the canonical way is is to be discussed I would say. Supporting find_package is actually not the most trivial thing and most libraries just don't. Most rely on outdated find Scripts and such stuff, I wouldn't call that the canonical way (I wouldn't call FetchContent the canonical way either, there is just none in my opinion).

Regarding your argument: If you install sqlpp11 on your system it will install date as well on your system. So no need to install it extra. That is one of the convenient things. (We could add a Component to the install here, so you can also decide to only install sqlpp11)

I think this might be also your misunderstanding. There is no need for you to use FetchContent as well in your own project. Just clone this lib, install it and it will install sqlpp11 and the dependencies (date in this case) as well. Quite convenient actually I would say. Then you can use sqlpp11 in your project just as you would normally. You need a newer version of date? Bump it and you good to go. You also have full control whether you want the lib and it's dependencies static or shared and such stuff.

Btw. you probably use a connector as well. In case you use sqlite3 this is even more convenient. Because the connector will FetchContent sqlpp11. So install the connector and you get everything.

That being said: A good argument against FetchContent might be that for some reasons you already have date installed and luckily it's already in the right version, so you don't want to build it again. I considered this to be not that likely (Date isn't a library that ships with package managers or such stuff), but I see that it's possible. And as I said above you can easily change it so it works for you. But it could make be simpler.

I propose we just leave users the (easy) choice and use a Flag called USE_SYSTEM_DATE (OFF by default). Set it and it will use find_package instead of FetchContent. That way we support both workflows with the default being the convenient way for most people. Would that be fine for you?

Nevertheless, you might wanna think if fetchContent is really not the right thing for you. I don't know your exact use case, but for most people this works fine very well and is a lot easier. (Just take the CI/CD as an example. You know need more lines than before. Now imagine that with 20 dependencies instead of one. And take also into account that date is one of the best libraries regarding CMake Support. It won't be so easy with all of them).
You gain a lot of flexibility to control what dependencies and in what versions and it what configuration you want them, right from your project. Need Library XYZ with some special configs, write it in your cmake. No need to write a huge DEVELOPER.md, where you have to explain everyone what libraries they have to install.
And while it's not perfect, you have a lot of options with FetchContent including providing a own path to local sources of a dependency or overriding versions and such stuff.
And it doesn't mean at all your end product cannot use shared libraries installed by the system or use a package manager. This is mostly for development. For installation you can do whatever fits you. FetchContent is comparable to git submodule in that regard, but a lot more flexible.
That being said, there is a case where you don't want FetchContent: For huge standard libraries, which are often part of the system anway. For instance Boost or QT.

kovdan01 · 2021-07-28T07:01:50Z

@Leon0402 Thank you for such a detailed answer! Adding USE_SYSTEM_DATE looks good to me - updated the PR, please see the changes.

Leon0402 · 2021-07-28T07:07:18Z

dependencies/CMakeLists.txt

@@ -22,11 +22,17 @@
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
 # SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

-include(FetchContent)



Are you sure this works? I'm not quite sure, but if I remember correctly find_package has to be called in the (parent) scope of where it's actually linked to.

That means find_package would need to be in the root CMakeFile (just as in your previous version and the if else has to be around the add_subdirectory(dependencies).

I'm not 100% sure though if I remember here correctly, I can't test it myself right now

Yes, I checked that it works on my system (Arch Linux x86-64, cmake 3.21.1) in both configurations: with USE_SYSTEM_DATE and without it (and it behaves as expected - uses find_package in the first case and fetches the library from github in the second one). Moreover, CI builds pass - it means that it works not only on my system :)

Regarding that:

find_package would need to be in the root CMakeFile

In my practice I put find_package in different level CMakeLists.txt files (trying to minimize the scope and use them only where needed), and never have problems. Perhaps your information is correct for some old cmake versions, but now using find_package and include anywhere looks OK.

Ah, sorry, my fault - everything was OK only with -DBUILD_TESTING=OFF (likely because sqlpp11 is header-only). You are right - date::date target is used in top-level CMakeLists.txt, that's why find_package should be located there too.

Leon0402 · 2021-07-28T07:11:50Z

dependencies/CMakeLists.txt

-    GIT_TAG        v3.0.0
-) 
+if(USE_SYSTEM_DATE)
+    find_package(Threads REQUIRED)


Why Threads here?

If we need this, then presumbably only on non msvc and you would also need to link against it. But I'm not sure if we do. cc: @rbock

It might be that wee need this and relied on Date for this https://github.com/HowardHinnant/date/blob/master/CMakeLists.txt#L152.
While this should work fine in both modes, it would be better to do this here as well if we need it to not rely on date providing it. But in these case we should find_package(Threads) in both cases I assume (and link against it)

I've added that in the first PR revision because it did not compile on my system without that. Now I've checked that it works correctly without it (and it is expected - date library marks Threads as a dependency, and it is found automatically: https://github.com/HowardHinnant/date/blob/master/cmake/dateConfig.cmake#L4). Deleting this line (as not needed).

kovdan01 · 2021-07-28T07:45:41Z

@Leon0402 Updated the PR based on your comments. Please see the changes

Leon0402 · 2021-07-28T07:51:42Z

CMakeLists.txt

+
+if(USE_SYSTEM_DATE)
+    find_package(date REQUIRED)
+endif()

 ### Dependencies 
 add_subdirectory(dependencies)


You should put the add_subdirectory here in the else I suppose :-) Otherwise you have not much gained, it will still fetch date.
And could you move the comment in line 44 "### Dependencies" above the if. That was there to introduce the section for dependencies, so the cmake file will be clean

You should put the add_subdirectory here in the else

Yes, but there is a little inconsistency here: in your case, if not USE_SYSTEM_DATE, we add_subdirectory(dependencies), which potentially might contain not only date dependency.

Otherwise you have not much gained, it will still fetch date.

It will not - I wrapped dependencies/CMakeLists.txt content into if(NOT USE_SYSTEM_DATE) block. I actually don't like this variant, but putting add_subdirectory(dependencies) in else branch looks a bit illogical too.

Maybe you could suggest an approach that will avoid the mentioned problems?

Ah right, sorry, I didn't see the other change. I think this is fine. Usually in my projects I use two ifs, one around the fetchContent_Declare and one around the add_subdirectory(). The reason is that with multiple dependencies you first specify all FetchContent_Declare and then all add_subdirectory / FetchContent_MakeAvailable, so common dependencies are only used one.

But as we only have one dependency right now, it doesn't matter I suppose. If there will be a second one we can split this up further.

Leon0402 · 2021-07-28T07:53:51Z

CMakeLists.txt

+if(USE_SYSTEM_DATE)
+    find_package(date REQUIRED)
+endif()

 ### Dependencies 
 add_subdirectory(dependencies)


Suggested change

if(USE_SYSTEM_DATE)

find_package(date REQUIRED)

endif()

### Dependencies

add_subdirectory(dependencies)

### Dependencies

if(USE_SYSTEM_DATE)

find_package(date REQUIRED)

else()

add_subdirectory(dependencies)

endif()

Just found out there is suggest changes button :-) But not tested, please check if it works

Leon0402

Seems fine to me @rbock , although I didn't test it myself (but pipelines runs, so should be alright)

rbock · 2021-07-29T05:37:03Z

Thanks both, for the intense discussion! Much appreciated.

* Add `USE_SYSTEM_DATE` cmake option

Massive refactor and cleanup release The changes below require git/build rules to be changed. I am expecting the result to be considerably simpler, though. It will also be much simpler to make changes to the library or individual connectors now (most of the misc changes below would have been much harder to do with the former multiple repos). Some of the misc changes below might break existing code in very rare cases (I am aware of exactly one). Happy to help fixing those :-) Connector libraries - Moved connector libraries for mysql/postgresql/sqlite3 into the sqlpp11 repository - Made connector libraries header-only - Added `USE_SYSTEM_DATE` cmake option (#372) Documentation: - Created docs directory (#364) - Removed wiki pages - Document multi_insert for time_point columns (#367) Fixes: - Several DateTime fixes for mysql - Add order_by and limit for mysql remove and update - Added shift left and shift right operators. - Added blob support for postgresql - Support MySQL connect timeout option Misc: - Replace serializer_t - Remove multi_column. - Remove tvin - Remove rhs_wrap - Remove _is_trivial from *_operand - Add is_equal_or_null(col, some_value_or_null) - Remove variations of serialization from interpretable - Remove null_is_trivial_value - Remove table::ref (see #345). - Remove same-name check for result columns

kovdan01 added 2 commits July 27, 2021 11:35

Do not use FetchContent for dependencies

b953660

Fix CI builds

2110f04

kovdan01 force-pushed the enhance_dependencies_resolution branch from c465b7b to 2110f04 Compare July 27, 2021 10:09

kovdan01 added 2 commits July 28, 2021 09:46

Revert changes

c814353

Add USE_SYSTEM_DATE cmake option

dc86ecb

Leon0402 suggested changes Jul 28, 2021

View reviewed changes

Fix PR comments

104076d

kovdan01 changed the title ~~Do not use FetchContent for dependencies~~ Add USE_SYSTEM_DATE cmake option Jul 28, 2021

Leon0402 suggested changes Jul 28, 2021

View reviewed changes

Leon0402 reviewed Jul 28, 2021

View reviewed changes

Leon0402 approved these changes Jul 28, 2021

View reviewed changes

rbock merged commit 77db534 into rbock:develop Jul 29, 2021

Leon0402 pushed a commit to Leon0402/sqlpp11 that referenced this pull request Dec 2, 2021

Add USE_SYSTEM_DATE cmake option (rbock#372)

3874577

* Add `USE_SYSTEM_DATE` cmake option

rbock mentioned this pull request Dec 18, 2021

Merge tag '0.61' into develop #402

Merged

Add USE_SYSTEM_DATE cmake option #372

Add USE_SYSTEM_DATE cmake option #372

Uh oh!

Conversation

kovdan01 commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbock commented Jul 28, 2021

Uh oh!

Leon0402 commented Jul 28, 2021

Uh oh!

kovdan01 commented Jul 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kovdan01 commented Jul 28, 2021

Uh oh!

Leon0402 Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Leon0402 left a comment

Choose a reason for hiding this comment

Uh oh!

rbock commented Jul 29, 2021

Uh oh!

Uh oh!

Add `USE_SYSTEM_DATE` cmake option #372

Add `USE_SYSTEM_DATE` cmake option #372

kovdan01 commented Jul 27, 2021 •

edited

Loading

Leon0402 Jul 28, 2021 •

edited

Loading