Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incompatibility in 3.19.0 #125

Closed
tdavis opened this issue May 23, 2017 · 10 comments
Closed

Incompatibility in 3.19.0 #125

tdavis opened this issue May 23, 2017 · 10 comments
Assignees
Labels

Comments

@tdavis
Copy link

tdavis commented May 23, 2017

Released all the way back in yesterday, and it's still broken! ;)

SQLite 3.19.0 is incompatible with this library thanks to a minor change to the main header file (sqlite3.h) as detailed here. I resolved this by making the same change to the forward declaration in Database.h. Unfortunately, I'm not sure how to fix it in a backwards-compatible fashion since having access to SQLITE_VERSION_NUMBER in Database.h would require including the header those declarations are there to avoid.

@SRombauts
Copy link
Owner

SRombauts commented May 24, 2017

Ouch!

Thanks for reporting this...

I think our best option is using CMake to detect the SQLite version and set a compile flag

@SRombauts
Copy link
Owner

Hey @tdavis, would you have some time to see if the version detection could be made through CMake ?

@sorinmustaca
Copy link

there is a very easy solution:
In Database.h just remove all those forward declarations and simply include the header.

// Forward declarations to avoid inclusion of <sqlite3.h> in a header
//struct sqlite3;
//struct sqlite3_context;
//struct Mem;
//typedef struct Mem sqlite3_value;

#include "sqlite3.h"

@SRombauts
Copy link
Owner

Thanks @sorinmustaca, that is in fact totally right, but my intent was to never include "sqlite.h" in headers to avoid "header leaks" in every application using my wrapper.

It compiles quicker in a big application if you avoid as much includes as possible.

Until today, it was working perfectly fine.

@sorinmustaca
Copy link

I don't think that there is a problem with this. Modern compilers can simply exclude the re-inclusion.
I would even dare to add a #pragma once there, because this is the "Microsoft" way to prevent re-inclusions.

And, if you look in the sqlite3.h, you see:
#ifndef SQLITE3_H
#define SQLITE3_H

...
#endif

So, multiple inclusions should be excluded...

@SRombauts
Copy link
Owner

Hi, thanks for the suggestion, I think I know very well how modern compilers work (I also use #pragma once in every header of the wrapper).

The problem is not with multiple inclusion of a "sqlite3.h" file: the problem is I have a 3600+ files application where I don't want those 1800 cpp files getting the inclusion to this "sqlite3.h" file.
=> when it was done like that, for sqlite3.h, windows.h, and many more, the compile time was way higher, because the compiler as to redo a similar job for each and every compile unit (it's not cached)!

I am not willing to de-optimized anything that was carefully designed, a simple solution will not do the job for my needs.

(also, I am using ccache an Unity Builds to reduce compile times)

@tdavis
Copy link
Author

tdavis commented May 24, 2017

I will look into doing the detection in CMake and get back to ya

@tdavis
Copy link
Author

tdavis commented May 24, 2017

So, the most straight forward way to accomplish this would be:

find_package(sqlite3)
if(sqlite3_VERSION VERSION_LESS "3.19")
    set_target_properties(SQLiteCpp PROPERTIES COMPILE_FLAGS "-DSQLITECPP_HAS_MEM_STRUCT")
endif()

Then just use that name in Database.h. Unfortunately, your current CMakeLists.txt doesn't support this approach; it either builds a local sqlite by default or just assumes it is available for linking without using find_package(). Having seen #111, using find_package() doesn't seem like your preference. I have modified the process locally to use hunter for the sqlite dependency as I have given up managing downstream dependencies (e.g. my library which depends on yours) without Hunter to curtail the insanity.

I am happy to supply a PR for this new version, but it will take some time to clean up and also get outstanding changes into Hunter's SQLite3 package which I've also modified locally (primarily for SQLITE_ENABLE_COLUMN_METADATA support.) Let me know your preference.

@SRombauts
Copy link
Owner

No, why not using find_package() if this solves the issue, I can change my mind depending on what is best (and still simple enough) for the users

@SRombauts
Copy link
Owner

SRombauts commented Jul 18, 2017

I tried to use a FindSQLite3.cmake file but it was not portable (Windows issues) so I followed on my previous choices to only rely on a new CMake variable SQLITE_USE_LEGACY_STRUCT that the user can set to enable a SQLITE_USE_LEGACY_STRUCT compile flag (OFF by default) to use with SQLite <3.19

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

No branches or pull requests

3 participants