search

martinling / libserialport

Unofficial personal repository for libserialport - see git://sigrok.org/libserialport for official repo
http://sigrok.org/wiki/Libserialport
GNU Lesser General Public License v3.0
65 stars 34 forks source link

Add CMake, MSVC support and fix memory/stack corruption. #28

Open jamesljlster opened 6 years ago

jamesljlster commented 6 years ago

CMake, MSVC Support

Now libserialport can be built natively under MSVC with cmake. The following files are created in order to have cmake support:

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.

glebm commented 5 years ago

@jamesljlster Can you please send the bugfixes & MSVC compat as a separate PR?

jamesljlster commented 5 years ago

Okay, I'll send stack corruption bugfix PR to https://github.com/sigrokproject/libserialport first. And then I will try to use a better way for cmake, MSVC support.

jamesljlster commented 5 years ago

@glebm @martinling Memory/stack corruption bugfix pull request had been sent to https://github.com/sigrokproject/libserialport/pull/1 Please check it out.

soul4soul commented 5 years ago

Thanks for the PR. I'm using it with cmake and ninja from VS2019.

martinling commented 4 years ago

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

martinling commented 4 years ago

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?

glebm commented 4 years ago

@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 (https://github.com/msys2/MINGW-packages/pull/4841) but it'd be nice to have a unified way to build it regardless of the target.

Conveniently, CMake has -G 'MSYS Makefiles' for msys2, -G 'MinGW Makefiles' for mingw (CMD), -G 'Visual Studio 15 2017 Win64', etc. CMake also has great IDE support, e.g. in VS Code and CLion.

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?

jamesljlster commented 4 years ago

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.

martinling commented 4 years ago

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

soul4soul commented 4 years ago

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.

martinling commented 4 years ago

@jamesljlster

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.

The way this works in autotools is:

Anyway - you don't need to try to process configure.ac or config.h.in with cmake. You just need to get cmake to implement the necessary system checks, and write a header that does the same job. It looks like you've already done most of that, which is great.

You can see how libserialport uses the definitions in config.h by looking at the places they appear in the C files. The main things that get used are some HAVE_<function/type> macros, the SP_API and SP_PRIV visibility prefixes, the NO_ENUMERATION and NO_PORT_METADATA macros which are set on less-supported operating systems.

There are some things in config.h.in which are not actually used. The package and library versions are now hard-coded into libserialport.h, because I needed to do that to make the VS2019 build work. There are also unused macros for the presence of common headers, such as HAVE_STDINT_H, which seem to be set automatically as a side effect of some other autoconf macro.

The use of _FILE_OFFSET_BITS is more subtle. This is a feature test macro used by the headers of glibc and some other C standard library implementations. If it is defined to 64 before any of the C library headers are included, then the headers will detect that and provide the 64-bit compatible versions of types used by stat etc, as opposed to legacy 32-bit versions.

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, _FILE_OFFSET_BITS is set by the AC_SYS_LARGEFILE macro which is one of autoconf's system services macros. That macro will also update C compiler flags accordingly on systems where that matters.

Presumably there's a way to do the same thing in cmake.

jamesljlster commented 4 years ago

@martinling It's possible to do large file test in cmake by writing custom compilation test: https://github.com/uclouvain/openjpeg/blob/master/cmake/TestLargeFiles.cmake

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.

martinling commented 4 years ago

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: https://git.savannah.gnu.org/cgit/autoconf.git/tree/lib/autoconf/specific.m4#n129

jamesljlster commented 4 years ago

Hello @martinling

I'm sorry for slow development due to the heavy works on my research. The new cmake build system could work on Linux now: https://github.com/jamesljlster/libserialport/tree/cmake_dev

Changes are:

But I'm facing some problem under msvc build. It seems that the most currently libserialport will only build with shared configuration under msvc? According to the macro logic in libserialport.h:

/** @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. Also due to single header limitation, static and shared library can't be existed in the same package folder. I'm wondering if it is better to revert the libserialport.h back to the version that would be generated from libserialport.h.in. Or I can just disable static library support under msvc?

ericoporto commented 3 years ago

Hey, I am interested in a CMake version of libserialport, did you solve this eventually?

martinling commented 3 years ago

@ericoporto I think @uwehermann had done some more work on this a while back - Uwe?