Do you think we should wait for this before making SOCI 4.1.0 release or can we leave this change until a later 4.1.x?

Do you think we should wait for this before making SOCI 4.1.0 release or can we leave this change until a later 4.1.x?

Well, it would probably be nice if 4.1 had this already, but it's more of a nice-to-have. So I guess you don't have to wait for this PR to be done.
However, my planned timeline for this PR is to get this finished within the next two to three weeks. Depending on your plans for 4.1, waiting for such a time period would be okay? But as I said: it's optional.

Unrelated to this PR, I would appreciate if #992 made it into 4.1 though :)

@vadz as the Windows CI demonstrates, CI will fail if I don't separate out the callback implementation. I think the most straightforward thing to do here would be to keep the separate implementation for this PR and then it can be removed after #1192 has been merged.

Otherwise, I believe I have addressed all remaining review comments 👀

But AFAICS you didn't remove SOCI_DECL for failover_callback? You have to do it if you inline these functions.

I won't have time to do it today, but if I can get back to this during the week-end: would you prefer to make a real merge or squash merge all the changes together (or something in between — but in this case you will need to do the rebase yourself)?

But AFAICS you didn't remove SOCI_DECL for failover_callback? You have to do it if you inline these functions.

No I didn't, because that's what #1192 already does and I didn't want to introduce partial mixing of PR changes.

would you prefer to make a real merge or squash merge all the changes together (or something in between — but in this case you will need to do the rebase yourself)?

I think given the magnitude of this PR, a real merge would be good as the commit history might turn out to be useful when blaming any of the touched code in the future (to get a feeling for why a given change was introduced).

But AFAICS you didn't remove SOCI_DECL for failover_callback? You have to do it if you inline these functions.

No I didn't, because that's what #1192 already does and I didn't want to introduce partial mixing of PR changes.

I'd rather do it here than add a new source file only to immediately delete it later. Any conflicts will just have to be resolved when applying the other PR later.

would you prefer to make a real merge or squash merge all the changes together (or something in between — but in this case you will need to do the rebase yourself)?

I think given the magnitude of this PR, a real merge would be good as the commit history might turn out to be useful when blaming any of the touched code in the future (to get a feeling for why a given change was introduced).

Yes, true, it's just that there are a lot of commits here and it seems like at least some of them are changed/replaced by later ones... But I guess we'll just have to live with this too.

I'd rather do it here than add a new source file only to immediately delete it later. Any conflicts will just have to be resolved when applying the other PR later.

Alright, will do 👍

Yes, true, it's just that there are a lot of commits here and it seems like at least some of them are changed/replaced by later ones... But I guess we'll just have to live with this too.

True. But sometimes this is the exact history of a changeset that one needs to see in order to understand why code lokks as it does today. So there is an upside as well :)

Thanks for the update!

I've tested this on my own system and I see a few (small?) problems:

1. The names of FIREBIRD_INCLUDE_DIR has changes to FIREBIRD_INCLUDE_DIRS, according to the documentation. I'd like to revert this because there seems to be no need to break the previously working builds using the existing variable name, but also because both this variable and FIREBIRD_LIBRARIES don't work anyhow: the actually working names are Firebird_INCLUDE_DIRS and Firebird_LIBRARIES. AFAICS this just needs to be renamed in cmake/find_modules/FindFirebird.cmake.
2. Configuration said Found MySQL: -L/usr/lib/x86_64-linux-gnu/ -lmariadb (found version "10.11.6") but the build failed due to mysql/mysql.h not being found and I only have /usr/include/mariadb/mysql.h here. I don't care much about MySQL, but the logic in cmake/find_modules/FindMySQL.cmake is wrong, it should define SOCI_MYSQL_DIRECT_INCLUDE if necessary when using CONFIG_EXE branch too (in fact, I'd rather drop SOCI_MYSQL_DIRECT_INCLUDE entirely and just set up the paths for #include <mysql.h> to always work but, again, I don't care enough about MySQL to feel strongly about this one way or the other).
3. The names of all tests have changed from soci_backend_test[_odbcdriver] to soci_backend[_odbcdriver]_tests. I can at least understand putting ODBC driver after the backend, but why change "test" to "tests"? I'd like to undo this too, I have a couple of scripts I use for running the tests for all the backends and I don't see why should they be changed and personally I also find "test" more suitable than "tests".

2. Configuration said Found MySQL: -L/usr/lib/x86_64-linux-gnu/ -lmariadb (found version "10.11.6") but the build failed due to mysql/mysql.h not being found and I only have /usr/include/mariadb/mysql.h here. I don't care much about MySQL, but the logic in cmake/find_modules/FindMySQL.cmake is wrong, it should define SOCI_MYSQL_DIRECT_INCLUDE if necessary when using CONFIG_EXE branch too (in fact, I'd rather drop SOCI_MYSQL_DIRECT_INCLUDE entirely and just set up the paths for #include <mysql.h> to always work but, again, I don't care enough about MySQL to feel strongly about this one way or the other).

Did some poking around and agree here; seems the MySQL header should be included directly with #include <mysql.h>.

On my (Ubuntu 22.04) I'm seeing the following from pkg-config --cflags mysqlclient:

-I/usr/include/mysql

The /usr/include/mysql directory contains mysql.h. Also other sample programs I've seen will include mysql.h directly, e.g. this example from the MariaDB docs as well as some others I've seen on the Internet.

I've noticed that both MySQL and MariaDB usage from C would directly #include <mysql.h> as MariaDB is supposed to still be compatible with MySQL for the most part post-fork. Probably because a proper MySQL program should also work with MariaDB; the client library and sever being used however would of course be different.