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

fixed libtest linker error on msvc #144

Closed
wants to merge 1 commit into from

Conversation

@snikulov
Copy link
Member

commented Mar 2, 2015

The issue:

first.obj : error LNK2019: unresolved external symbol snprintf referenced in function hexdump [\curl\b\tests\libtest\lib526.vcxproj]
  C:\WORK\GITHUB\dev\curl\b\tests\libtest\Release\lib526.exe : fatal error LNK1120: 1 unresolved externals [\curl\b\tests\libtest\lib526.vcxproj]

How to reproduce:

cmake .. -G"Visual Studio 11 Win64"
cmake --build . --config Release

Suggested solution:
mprintf.h header added to test.h to prevent future issues in libtest

@gvanem

This comment has been minimized.

Copy link
Member

commented Mar 2, 2015

IMHO, adding -Dsnprintf=_snprintf to the CFLAGS would be cleaner. I don't know how since CMake is gibberish to me.

@snikulov

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2015

@gvanem according to documentation _snprintf has different semantic from snprintf (for example http://stackoverflow.com/questions/7706936/is-snprintf-always-null-terminating/13067917#13067917). If this not important I can do the trick with little update in CMakeLists.txt

Waiting for more comments.

@gvanem

This comment has been minimized.

Copy link
Member

commented Mar 2, 2015

I don't think snprintf() vs. _snprintf() matters for libtests/*.c files.

@bagder

This comment has been minimized.

Copy link
Member

commented Mar 2, 2015

Your patch removes the use of the internal snprintf(), why does that solve the problem? Why shouldn't the windows built versions also use that?

@snikulov

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2015

@bagder it not removes. I include it in single place test.h and remove duplication.

@bagder

This comment has been minimized.

Copy link
Member

commented Mar 2, 2015

Ah, thanks for clearing that out. I was clearly not reading the whole patch. With me being corrected. I can't see any problems with this patch!

@bagder

This comment has been minimized.

Copy link
Member

commented Mar 2, 2015

Oh, unless of course you build with CURLDEBUG defined. It shows we have some further cleaning up work to do... (use of sprintf() that we frown upon)

@snikulov

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2015

Ok. Will check it soon. Perhaps I'll need also update cmake build to honor CURLDEBUG option, because I'm not currently aware of that.

@bagder

This comment has been minimized.

Copy link
Member

commented Mar 2, 2015

Ah right. With the configure build you use --enable-debug to switch it on, and it enables extra debugging info, checks and outputs in various way.

@snikulov

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2015

Well, some clarification --enable-curldebug - enables TrackMemory feature, while --enable-debug - enables both (Debug && TrackMemory). Am I correct?

Another question - should it be wirtten to curlbuild.h or as autoconfig do - just set definition to Makefiles?

@snikulov

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2015

I've submitted another pull request #145 to handle CURLDEBUG define for build in cmake.

@snikulov

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2015

@bagder what should we do, when CURLDEBUG enabled?
Code from mprintf.h will definetely break the build

#ifdef CURLDEBUG
/* When built with CURLDEBUG we define away the sprintf functions since we
don't want internal code to be using them */
# define sprintf sprintf_was_used
# define vsprintf vsprintf_was_used
#else
# define sprintf curl_msprintf
# define vsprintf curl_mvsprintf
#endif
@bagder

This comment has been minimized.

Copy link
Member

commented Mar 3, 2015

I think that since curl/mprintf.h is a public header, it should not feature that #ifdef in the first place. I think I'll help out here and move that debug-#define sequence into a private libcurl header instead.

bagder added a commit that referenced this pull request Mar 3, 2015
@bagder

This comment has been minimized.

Copy link
Member

commented Mar 3, 2015

Thanks, I cleaned up the include thing first, then merged your patch!

@bagder bagder closed this Mar 3, 2015

@snikulov snikulov deleted the snikulov:libtest_windows_build branch Mar 4, 2015

@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.