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.

Thanks for the feedback! With regards to the MySQL situation I have checked on my system again and indeed PkgConfig reports -I/usr/include/mysql as well. Using -I/usr/include with #include <mysql/mysql.h> just happened to work by chance. I have revised the corresponding Find module and dropped the SOCI_MYSQL_DIRECT_INCLUDE dance entirely as the find module should now always pick the correct include path 👀

Furthermore, I have adapted the module to respect externally set MySQL_INCLUDE_DIRS etc. variables (unless MySQL is found via vcpkg or pkg-config) instead of overwriting them.

I'd like to revert this because there seems to be no need to break the previously working builds using the existing variable name

I don't revert the change as the way it works now is the canonical way how things work in most other CMake find modules. However, I have added a compatibility layer that will automatically translate between the old FIREBIRD_INCLUDE_DIR and the new Firebird_INCLUDE_DIRS so old builds should remain functional.

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.

Good catch! I'll update the docs accordingly (the same is true for Oracle and MySQL).

The names of all tests have changed from soci_backend_test[_odbcdriver] to soci_backend[_odbcdriver]_tests.

Whoops - I thought I had reverted all name changes. I'll do so now.

I have a couple of scripts I use for running the tests for all the backends

I'd recommend simply using ctest for this purpose (potentially with the -R option to restrict to a specific subset). That way you are independent from any specific names of the executables.

To my knowledge I am up-to-date again in terms of addressing review comments. Let me know if I missed something!

Finally, I have taken the liberty of adding a commit that updates the used FreeBSD version during CI to 14.2 as the 14.0 image is no longer available and therefore leads to the CI failing. If you prefer of doing this separately, I can simply remove that commit again but I was hoping that this PR gets merged soon-ish anyway and therefore it shouldn't matter.

@vadz I remember that you asked me whether I have fixed #1008 in this PR. I can now say that I did not. The reason that CI passes is that I have disabled ASAN for the time being for ODBC.

Thanks for the feedback! With regards to the MySQL situation I have checked on my system again and indeed PkgConfig reports -I/usr/include/mysql as well. Using -I/usr/include with #include <mysql/mysql.h> just happened to work by chance. I have revised the corresponding Find module and dropped the SOCI_MYSQL_DIRECT_INCLUDE dance entirely as the find module should now always pick the correct include path 👀

Great, thanks!

I'd like to revert this because there seems to be no need to break the previously working builds using the existing variable name

I don't revert the change as the way it works now is the canonical way how things work in most other CMake find modules. However, I have added a compatibility layer that will automatically translate between the old FIREBIRD_INCLUDE_DIR and the new Firebird_INCLUDE_DIRS so old builds should remain functional.

OK, this works too, thanks.

I have a couple of scripts I use for running the tests for all the backends

I'd recommend simply using ctest for this purpose (potentially with the -R option to restrict to a specific subset). That way you are independent from any specific names of the executables.

Thanks, this is probably a good advice, but I don't always CMake for building and I also suspect that I might be not the only one doing it like this, so it's still nice to avoid unnecessary changes.

To my knowledge I am up-to-date again in terms of addressing review comments. Let me know if I missed something!

I'll try to retest a.s.a.p., thanks again for your work!

Finally, I have taken the liberty of adding a commit that updates the used FreeBSD version during CI to 14.2 as the 14.0 image is no longer available and therefore leads to the CI failing. If you prefer of doing this separately, I can simply remove that commit again but I was hoping that this PR gets merged soon-ish anyway and therefore it shouldn't matter.

Yes, it doesn't really matter and I do hope to merge it soon.