Disable explicit setting of MSVC runtime by emmenlau · Pull Request #236 · SRombauts/SQLiteCpp (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation12 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This PR removes the explicit setting of the MSVC runtime from SQLiteCpp. This is because overriding the default runtime can have various negative implications. For example, the build of SQLiteCpp fails on MSVC 2019 with errors:
LINK : warning LNK4098: defaultlib 'MSVCRTD' conflicts with use of other libs; use /NODEFAULTLIB:library
LINK : warning LNK4217: symbol 'free' defined in 'libucrtd.lib(free.obj)' is imported by 'sqlite3.lib(sqlite3.c.obj)' in function 'vfsUnlink'
LINK : warning LNK4217: symbol 'malloc' defined in 'libucrtd.lib(malloc.obj)' is imported by 'sqlite3.lib(sqlite3.c.obj)' in function 'vfsUnlink'
LINK : warning LNK4217: symbol '__acrt_iob_func' defined in 'libucrtd.lib(_file.obj)' is imported by 'sqlite3.lib(sqlite3.c.obj)' in function 'sqlite3VdbePrintOp'
LINK : warning LNK4217: symbol 'fflush' defined in 'libucrtd.lib(fflush.obj)' is imported by 'sqlite3.lib(sqlite3.c.obj)' in function 'sqlite3VdbePrintOp'
LINK : warning LNK4217: symbol '__stdio_common_vfprintf' defined in 'libucrtd.lib(output.obj)' is imported by 'sqlite3.lib(sqlite3.c.obj)' in function '_vfprintf_l'
LINK : warning LNK4217: symbol 'strcspn' defined in 'libucrtd.lib(strcspn.obj)' is imported by 'sqlite3.lib(sqlite3.c.obj)' in function 'patternCompare'
LINK : warning LNK4217: symbol 'strncmp' defined in 'libucrtd.lib(strncmp.obj)' is imported by 'sqlite3.lib(sqlite3.c.obj)' in function 'sqlite3VListNameToNum'
LINK : warning LNK4217: symbol '_wassert' defined in 'libucrtd.lib(assert.obj)' is imported by 'sqlite3.lib(sqlite3.c.obj)' in function 'sqlite3CompileOptions'
sqlite3.lib(sqlite3.c.obj) : error LNK2019: unresolved external symbol __imp__msize referenced in function vfsUnlink
sqlite3.lib(sqlite3.c.obj) : error LNK2019: unresolved external symbol __imp_realloc referenced in function vfsUnlink
sqlite3.lib(sqlite3.c.obj) : error LNK2019: unresolved external symbol __imp__localtime64_s referenced in function localtime_s
sqlite3.lib(sqlite3.c.obj) : error LNK2019: unresolved external symbol __imp__beginthreadex referenced in function sqlite3ThreadCreate
sqlite3.lib(sqlite3.c.obj) : error LNK2019: unresolved external symbol __imp__endthreadex referenced in function sqlite3ThreadProc
SQLiteCpp_example1.exe : fatal error LNK1120: 5 unresolved externals
This error is resolved by the PR.
Coverage remained the same at 100.0% when pulling 521181c on BioDataAnalysis:emmenlau_remove_msvc_runtime_setting into a99d48d on SRombauts:master.
Hello @emmenlau , have you seen that this is breaking all Visual Studio build variants on AppVeyor CI?
I also tried on my new 3.x branch with updated Googletest and I get the same link issue.
I am pretty sure it's something that could be fixed with a better configuration of Googletest, but I lack the time to dig the issue at the moment.
Cheers!
I've significantly modified this PR. I understand now that the googletest shipped with SQLiteCpp is build static by default, and therefore links the static runtime libraries by default on MSVC. I was not aware of this before. Instead of just removing the static runtime setting on MSVC, I have left it the default.
But users building a shared SQLiteCpp and linking against a shared googletest may prefer to set the (newly added) cmake option -DSQLITECPP_USE_STATIC_RUNTIME=OFF in order to avoid linker problems about mixed runtime libraries.
Please review.
Hello, I think we should keep this options effects inside the "build with tests" branches
By which I mean have effects only on builds with Google test (which is not the default build)
Right now, you are enforcing static linking by default to everyone
Cheers!
That was actually by intention, because I realized that having such a setting for the type of runtime is generally useful (and not uncommon) on the MSVC architecture. For example this setting also in google test or glfw to name just a few.
But its really your call! Do you prefer to leave the setting for test only, or make it generally available?
BTW, another idea would be to change google test from using static MSVC runtime to dynamic runtime by default. Was there a good reason to use the static runtime? I know that this is the google test default, but I never understood why...
Well, I don't remember why they use static by default (there are multiple good reason in facts, like exceptions don't work well accross dynamic libraries IIRC)
But I agree that the best thing would be to keep your new option and configure Google test to use the same linkage
It's been a while since I looked into this... can the PR already move forward? Thanks a lot for your consideration!
Sorry if I appear a bit pushy, but it will soon be one year since I started this PR - can we get it over the finish line?
Anything I can do to help this move forward?
Thank you again @emmenlau
I am very sorry on how I treated your pull request and this very repository, there is no good excuse.
Just know that in the span of a year, I have had my third kid, then change my work position, and now just moved to a new house right before new lockdown time in France, so I mostly just ignored all external notifications. It should now only improve in the coming monthes
Please accept my apologies!
Dear @SRombauts oh my, congratulations to the new child and all the best to the happy family!!!! I'm terribly sorry for being so pushy, often its not easy to look into other peoples domain but I'm very glad for you! Be safe and hope everyone is well!
But please keep pushing, this is the kind of reminder I need to get back on tracks :)