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

Disable explicit setting of MSVC runtime #236

Conversation

emmenlau
Copy link
Contributor

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.

@coveralls
Copy link

coveralls commented Dec 14, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 521181c on BioDataAnalysis:emmenlau_remove_msvc_runtime_setting into a99d48d on SRombauts:master.

@SRombauts SRombauts self-assigned this Dec 26, 2019
@SRombauts
Copy link
Owner

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!

@emmenlau emmenlau force-pushed the emmenlau_remove_msvc_runtime_setting branch from 234e9ac to 521181c Compare January 10, 2020 14:01
@emmenlau
Copy link
Contributor Author

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.

@SRombauts
Copy link
Owner

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!

@emmenlau
Copy link
Contributor Author

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...

@SRombauts
Copy link
Owner

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

@emmenlau
Copy link
Contributor Author

It's been a while since I looked into this... can the PR already move forward? Thanks a lot for your consideration!

@emmenlau
Copy link
Contributor Author

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?

@emmenlau
Copy link
Contributor Author

Anything I can do to help this move forward?

@SRombauts SRombauts merged commit 3e35404 into SRombauts:master Nov 18, 2020
@SRombauts
Copy link
Owner

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!

@emmenlau
Copy link
Contributor Author

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!

@SRombauts
Copy link
Owner

But please keep pushing, this is the kind of reminder I need to get back on tracks :)

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

Successfully merging this pull request may close these issues.

3 participants