-
Notifications
You must be signed in to change notification settings - Fork 34
Add CMake, MSVC support and fix memory/stack corruption. #28
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
base: master
Are you sure you want to change the base?
Conversation
Fix cmake MSVC test compile flag. Add SP_API function declaration prefix.
|
@jamesljlster Can you please send the bugfixes & MSVC compat as a separate PR? |
|
Okay, I'll send stack corruption bugfix PR to https://github.com/sigrokproject/libserialport first. |
|
@glebm @martinling |
|
Thanks for the PR. I'm using it with cmake and ninja from VS2019. |
|
Hi @jamesljlster, Thanks for this PR - I'm sorry it has taken a long time to respond to, there was not much development happening on this project for a while. I discussed the proposed CMake support with @uwehermann and we were concerned that having two copies of various build logic in two different build systems (autotools and cmake) would be problematic in terms of support and maintenance. The other option would be to switch entirely to CMake, but at this point we have a number of downstream users with packaging scripts etc that depend on the existing autotools setup. In terms of improving support for native development on Windows, we felt it might be better to just directly include project files in the package that could be used with MSBuild or Visual Studio. There is now a branch with this done here: https://github.com/martinling/libserialport/commits/vs2019 - I would be very interested in feedback from VS users on this. That branch includes a lot of fixes for MSVC compatibility, both for the errors addressed in this PR and a lot of further build warnings and minor issues. The wc_to_utf8 bug was fixed separately, see https://sigrok.org/bugzilla/show_bug.cgi?id=1031 |
|
I would add that I could be convinced otherwise on including CMake support, especially given that libserialport doesn't change much - but I can't speak for @uwehermann, who has to maintain the package even when I disappear for long periods. My question to interested users (including @glebm and @soul4soul here) would be: does having a Visual Studio project supplied in the package meet your needs, or do you specifically want to be able to generate this through CMake? |
|
@martinling When I was using it, I wanted to be able to easily build it under msys2 and Visual Studio. I did msys2 using autotools (msys2/MINGW-packages#4841) but it'd be nice to have a unified way to build it regardless of the target. Conveniently, CMake has With CMake you get support for a lot of platforms for free, built-in support for cross-compilation, etc. I think it'll be easier for @uwehermann to maintain stuff with CMake rather than autotools. Autotools support can just be removed. I don't know what packages you maintain but they likely all support CMake already? |
|
Hello @martinling, Just like @glebm said. cmake is really powerful tool for development. It save me a lot of time in project platform transferring. When updating project architecture, I just need to modify cmake files instead of editing multiple build files such as Makefile or IDE project file. I've checked the vs2019 branch. It seems to be more friendly to add cmake build files (less or none of files edited) and keep autotools files. But I get stuck in generating config.h in cmake from configure.ac. I would like to know how config.h.in is generated by autotools and how config.h is needed by libserialport. |
|
@glebm the issue isn't with packages we maintain, it's with downstream users of the library. There are a lot of people using libserialport as a dependency in projects that have nothing to do with us, and not all of them are public. This is a library with a stable, published API/ABI and build procedure. That means that anyone should be able to put any new release of the package in place of an old one and have everything work. We take that very seriously, which requires us to be pretty conservative about those aspects of the package. We have to be a lot more fussy about this than accepting code changes - if we introduce a bug, that's a shame, but it happens and we can fix it in a newer version and everything will work again. But if we release a version that screws up backwards compatibility, that makes a huge mess that's much harder to resolve. So if we were to add CMake as a build option it would need to exist in parallel, and then we'd need to maintain it going forward. Before merging it, we'd need to be sure that the libraries it builds are fully equivalent to those built from autotools, across different platforms, and make sure that stays the case in future - or we end up with a support nightmare from all the possibilities. And if we wanted to drop autotools, we'd need to support both systems for one or more releases and display warnings that the autotools build is deprecated. Part of the testing for the VS2019 support I have just added was to ensure that DLLs built by autotools/MinGW-w64 and MSBuild/VC++ were safely interchangeable. That was manageable for Windows on x86/x64 with a single version of Visual Studio, but gets a lot harder to check for all the possible platforms that the library can be built for with autotools, let alone all the different build systems that can be targeted by CMake. I do see the need to improve people's ability to use the library as part of different development workflows though, and create native builds easily on their preferred platforms. And I see the advantages of CMake for that, which is why I said I'm potentially open to doing this. But it's not something we can do lightly, it needs a lot of care and planning. |
|
Having a VS project supplied by libserialport meets my primary needs. I was most interested in this PR since it simplifies building on Windows without mingw dependency. The fact that it used CMake was a plus. Projects built by my company have been slowly switching to CMake for the build system. Although I haven't had any issues with libserialport since we began using it, I'm glad to see more development work being put into libserialport. There is value in being able to run a mirror directly without patches. |
42c7d87 to
7b0686e
Compare
The way this works in autotools is:
Anyway - you don't need to try to process You can see how libserialport uses the definitions in There are some things in The use of We don't actually access large files in libserialport, but we want to avoid the use of legacy APIs which maintainers are gradually trying to deprecate and remove. In the autotools version, Presumably there's a way to do the same thing in cmake. |
|
@martinling I think all checks about compilation could be done with compilations. If cmake does not provide official method for the test, just write a custom compilation test in cmake. |
|
Yes that approach should be fine, and it looks like that's the basis of the autoconf version - the source for that macro is here: |
|
Hello @martinling I'm sorry for slow development due to the heavy works on my research. Changes are:
But I'm facing some problem under msvc build. /** @cond */
#ifdef _MSC_VER
/* Microsoft Visual C/C++ compiler in use */
#ifdef LIBSERIALPORT_MSBUILD
/* Building the library - need to export DLL symbols */
#define SP_API __declspec(dllexport)
#else
/* Using the library - need to import DLL symbols */
#define SP_API __declspec(dllimport)
#endif
#else
/* Some other compiler in use */
#ifndef LIBSERIALPORT_ATBUILD
/* Not building the library itself - don't need any special prefixes. */
#define SP_API
#endif
#endif
/** @endcond */If we want to build static library, SP_API should leave it blank. |
|
Hey, I am interested in a CMake version of libserialport, did you solve this eventually? |
|
@ericoporto I think @uwehermann had done some more work on this a while back - Uwe? |
CMake, MSVC Support
Now libserialport can be built natively under MSVC with cmake.
The following files are created in order to have cmake support:
Implement some autoconf checking.
Let cmake find_package() can handle libserialport.
Modify from config.h.in created by autoconf.
Modify from libserialport.h.in.
During configuration, cmake will generate config.h and libserialport.h from config.h.cmake and libserialport.h.cmake.
But some tests are not performed:
I don't know very much about autoconf.
Maybe there are some important checks I have not implemented.
CMake build have been tested under the following platform:
MinGW-w64 x86_64 7.3.0 (MSYS2, Windows 10)
MinGW-w64 x86_64 6.3.0 (Windows 10)
Visual Studio 14 2015 Win64, MSVC 19.0 (Windows 10)
GCC x86_64 5.4.0 (Ubuntu 16.04.4 LTS)
Bug fix: memory/stack corruption
In windows.c: static char *wc_to_utf8(PWCHAR wc_buffer, ULONG size), wc_str initialized with insufficient size.