Revamp CMake support - Githubissues

SOCI / soci

Official repository of the SOCI - The C++ Database Access Library

http://soci.sourceforge.net/

Boost Software License 1.0

1.41k stars 477 forks source link

Revamp CMake support #1118

Open Krzmbrzl opened 9 months ago

Krzmbrzl commented 9 months ago

Fixes #1115 Fixes #1152 Fixes #1122 Fixes #1094

vadz commented 8 months ago

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?

Krzmbrzl commented 8 months ago

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 :)

Spixmaster commented 3 months ago

@vadz This pull request is probably needed for 4.1 to work. See #1153.

vadz commented 3 months ago

As I wrote in #1153, the header really should be renamed/moved, so it's not directly related to this PR, but please let me know when this one will be ready for review in any case. Thanks!

Krzmbrzl commented 1 month ago

@vadz Now that I finally worked around the VS2015 bug, I think this PR is ready for review

@papoteur-mga I'd appreciate your continued feedback on this too :)

vadz commented 1 week ago

I'd really like to refactor the tests to avoid having to recompile the entirety of common-tests.h several times (at least once for each backend and more for ODBC) when building them, but the changed needed to do it risk conflicting with this PR.

Not sure if I should wait for it to be merged or should make these changes and resolve the conflicts later.

Krzmbrzl commented 1 week ago

So effectively you want to do the refactoring that I have already done here? 👀

Personally, I would tend to not create the same change in a different PR while we are already working on getting this PR merged :shrug:

vadz commented 1 week ago

Yes, I'd like to apply this part (which is, again, something I really wanted to do since a long time, so thanks for finally doing it), except

Without test-foo.cpp renaming to foo_tests.cpp which, IMO, doesn't have enough benefits to compensate for inconveniences.
Without waiting until all the other issues elsewhere can be resolved which risks taking some time.

Would it be possible to do this somehow?

Krzmbrzl commented 1 week ago

compensate for inconveniences.

Which inconveniences?

Would it be possible to do this somehow?

I guess, but it would be a bit fiddly. Not sure if I personally see the value in having this part of the change a bit earlier given that the current approach has been used for so long (so my question here is: why the sudden hurry?) :shrug:

vadz commented 1 week ago

compensate for inconveniences.

Which inconveniences?

Renaming the file complicates its history (even if Git is much better at this than many other VCS). And the real question is anyhow not "why not do it" but "why do it", i.e. do we have any good reason for renaming these files? And I just don't see any, both test-foo and foo_tests are pretty much equivalent and the choice between them seems just a matter of personal preferences (and FWIW I do prefer using - to _).

Would it be possible to do this somehow?

I guess, but it would be a bit fiddly. Not sure if I personally see the value in having this part of the change a bit earlier given that the current approach has been used for so long (so my question here is: why the sudden hurry?) 🤷

No particular hurry, but I'm thinking about this every time I have to modify the tests and I plan on doing this again soon, so I thought it would be nice to finally get it done.