Minimum required patches for building ncurses libraries on Visual Studio

[madebr]

These patches are the minimum required patches to let me build the ncurses libraries on Windows using Visual Studio.

I created these while trying to package ncurses 6.2 for conan.
My WIP scripts can be found at https://github.com/madebr/conan-center-index/tree/ncurses/recipes/ncurses/all

Building for Visual Studio was causing some problems because it is missing some features.

This patchset adds workarounds for building the ncurses libraries.

What these patches do not fix is biuilding shared ncurses shared libraries (dlls) on Windows.
NCURSES_IMPEXP is defined globally and is the same for every ncurses sublibrary.
Whereas each sublibrary must have its own definition.
What I mean is that when building libmenu, a preprocessor variable LIBMENU_IMPEXP must be __declspec(dllexport) but NCURSES_IMPEXP must be __declspec(dllimport).

[ThomasDickey]

The NOMACROS change will cause the library to not work...

[madebr]

@ThomasDickey
I've got to admit that I did not give much attention to fixing the errors in that patch.
I'll rebase my paches on current master and see how things changed.

[ThomasDickey]

I haven't tested with msvc yet, but current code seems to work with mingw. Compiler warning made me notice an error in one memcpy (fixed), but the rest of my changes for win_driver.c were to reduce compiler warnings. So I don't think there'll be much surprise - just something to review, etc.

[madebr]

I've updated the patches.
Windows CI output at https://ci.appveyor.com/project/madebr/ncurses/builds/31521971

The NOMACROS patch is still there because of an error.
Please read the commit message to see what error is thrown.
6e407c1
Maybe you can immediately see what's wrong.

The same goes for the other error:
a31f132

[ThomasDickey]

yes, I'm aware of this, but the lib_gen.c change will not produce a working library (just something that compiles). I've been setting up a development environment to work on this, but am not done.

[ThomasDickey]

Defining NCURSES_NOMACROS will allow it to compile, but the library will not work, because calling those functions will result in an infinite recursion on the stack.

[madebr]

Things these patches fix:

• for shared libraries, use a define for each library
  In CMake terminology, this is using DEFINE_SYMBOL + dllimport/dllexport.
  This is done to e.g. avoid the form library to export a function, defined in ncurses.
• linking the progs requires an extra patch (new patch)
• make it compile by fixinglib_gen.c by modifying the regex in ncurses/base/MKlib_gen.sh.
  This is done to make it compile on MSVC

appveyor ci: https://ci.appveyor.com/project/madebr/ncurses/builds/33060824

TODO (What is not fixed/untouched)

• lib_gen.c
• on make install, tic fails to build the terminfo databases

The output is:

/bin/sh ./run_tic.sh
** Building terminfo database, please wait...
Running tic to install C:/Users/maarten/.conan/data/ncurses/6.2-20200516/_/_/package/911e47335817cd75de61d9b47c4d68a137801d08/share/terminfo ...

  You may see messages regarding extended capabilities, e.g., AX.
  These are extended terminal capabilities which are compiled
  using
          tic -x
  If you have ncurses 4.2 applications, you should read the INSTALL
  document, and install the terminfo without the -x option.

ncurses 6.2.20200516
"terminfo.tmp", line 1115, col 36, terminal 'fbterm': limiting value of `pairs' from 0x10000 to 0x7fff
"terminfo.tmp", line 4839, col 36, terminal 'xterm+256color': limiting value of `pairs' from 0x10000 to 0x7fff
"terminfo.tmp", line 4858, col 36, terminal 'xterm+256setaf': limiting value of `pairs' from 0x10000 to 0x7fff
"terminfo.tmp", line 4902, col 25, terminal 'xterm+direct2': limiting value of `colors' from 0x1000000 to 0x7fff
"terminfo.tmp", line 4902, col 40, terminal 'xterm+direct2': limiting value of `pairs' from 0x10000 to 0x7fff
"terminfo.tmp", line 4917, col 25, terminal 'xterm+direct': limiting value of `colors' from 0x1000000 to 0x7fff
"terminfo.tmp", line 4917, col 40, terminal 'xterm+direct': limiting value of `pairs' from 0x10000 to 0x7fff
"terminfo.tmp", line 4939, col 25, terminal 'xterm+indirect': limiting value of `colors' from 0x1000000 to 0x7fff
"terminfo.tmp", line 4939, col 40, terminal 'xterm+indirect': limiting value of `pairs' from 0x10000 to 0x7fff
"terminfo.tmp", line 7710, col 36, terminal 'dvtm-256color': limiting value of `pairs' from 0x10000 to 0x7fff
? tic could not build C:/Users/maarten/.conan/data/ncurses/6.2-20200516/_/_/package/911e47335817cd75de61d9b47c4d68a137801d08/share/terminfo

[ThomasDickey]

Referring to the packaging_patches branch, some of those have been addressed:

• Fix lib_gen.c (though I recall that one of the later patches relies upon the syntax change)
• Add include file to add export annotation

These are the ones that I'm considering:

• Pass BUILDING_XXX compile definition while building library XXX
• Let each sublibrary do its own import/exporting

The problem that I see in these is that they change longstanding public-use header files, making them dependent upon a different build configuration. I'm considering how to solve the problem without breaking compatibility.

This patch is solving a problem which probably should be done outside ncurses:

• Enable building progs using shared MSVC

These don't look suitable for inclusion in ncurses:

• Added .gitignore
• Add appveyor CI

[madebr]

Referring to the packaging_patches branch, some of those have been addressed:

* [Fix lib_gen.c ](https://github.com/madebr/ncurses/commit/effaed661e964f5fe3cb777e6bdb5f5450a77cb8) (though I recall that one of the later patches relies upon the syntax change)

See c976a90#diff-f425a63d0f18f4bbd666c1086bac3b6d for the other change.
The MKlib_gen.sh script requires 2 changes in the 6.2 release.

The problem that I see in these is that they change longstanding public-use header files, making them dependent up a different build configuration. I'm considering how to solve the problem without breaking compatibility.

Using MSVC, these changes can be avoided by using module-definition (.def) files.
See https://docs.microsoft.com/en-us/cpp/build/exporting-from-a-dll-using-def-files?view=vs-2019
This would require listing all exported symbols for ncurses, and for all its sublibraries, in a file.
But this is not really advised, and bug prone because symbols can be missed easily.

See https://www.gnu.org/software/gnulib/manual/html_node/Exported-Symbols-of-Shared-Libraries.html#Exported-Symbols-of-Shared-Libraries,
where the same process as I have used in these patches is described.

This patch is solving a problem which probably should be done outside ncurses:

* [Enable building progs using shared MSVC ](https://github.com/madebr/ncurses/commit/b24a45d3c622f14f7720e2b01327ba3a33a1a711)

Yes, because this patch creates a script that will link object files to an executable.
No, because Makefile also passes LDFLAGS (such as -link -dll that are meant only for linking dll's).
I don't see how I can externally override LINK_PROGS if this script should be kept outside of ncurses.

These don't look suitable for inclusion in ncurses:

* [Added .gitignore ](https://github.com/madebr/ncurses/commit/558ec611e1beb08050a7648c1999eddf84e05b68)

* [Add appveyor CI ](https://github.com/madebr/ncurses/commit/cf9de8bdf21c4771d939bc6d92cdec20ffbdf546)

I totally agree.
I added these as a proof that these patches make ncurses compile on MSVC.
(please continue to ignore these)

[ThomasDickey]

hmm - at the moment I'm test-building the mingw stuff, to review, etc. (I didn't find time to test with msvc, too many distractions recently).

[ThomasDickey]

Today I tried the full set of changes, noticed redefinition warnings (in the import/export stuff), backed out those changes for now, but added the aclocal.m4 change and amended the lib_gen.c change (the newline substitution isn't portable, but the parentheses don't appear to be needed now).

[madebr]

Cool!
On what os/compiler + build type are these warnings appearing?
I only tested the common combinations, available in the conanfile: normal and shared.

While you're looking at exporting symbols on mingw,
I think these patches will break building a dll using libtool because NCURSES_STATIC is defined for it. I don't know how to make CFLAGS dependent on the shared'ness of the library using libtool.

[ThomasDickey]

I was using gcc-10 on Debian/testing. Regarding libtool, I have a different to-do (fixing build problems with the gnat/Ada95), but I mostly build without libtool. However I have a scripted build for that configuration which I work on. The NCURSES_STATIC does look like a potential problem, but at the moment I was seeing how to reduce this backlog a little (there's also the two bug reports that I'm working on...).

[madebr]

(there's also the two bug reports that I'm working on...).

Please don't feel obligated to work under a deadline to fix this problem..
I'm just posting my patches and findings here so they aren't lost and hopefully end up in ncurses.

Through debugging, I found out what caused the tic.exe problem, described in #1 (comment).
The access function by MSVC only accepts access_mode flags where R_OK and/or W_OK is set.
The assertion (access_mode & (~6)) == 0 fails. (R_OK | W_OK == 6)
So the flag X_OK is the cause of this issue.

The msdn documentation for access only describes R_OK and W_OK.
It also describes the function does not work on directories.

Looking at the definition of access, it looks like its usage is discouraged. So perhaps its use should be reduced (or even removed).

Patching out the use of X_OK in a bad patch, the progs build and install fine on Windows.
https://ci.appveyor.com/project/madebr/ncurses/builds/33162174

[ThomasDickey]

For access, I'd wrap that in a macro (as for instance done for binary-mode fopen). I'm not working this item on a deadline, but keep in mind that things that aren't active tend to go onto a continually-growing to-do list :-)

[madebr]

I changed access to a macro.
https://ci.appveyor.com/project/madebr/ncurses/builds/33180879

In #1 (comment), you were talking about errors.
I just tried building ncurses with ../configure --with-shared --without-normal --without-debug --enable-fvisibility and then indeed linking the progs fail because they are using private functions from the ncurses library.
Removing --enable-fvisibility makes the library build and link fine (using the configure flags above).

[ThomasDickey]

In today's patch (20200704), I'm adding most of this, except:

• the content of "ncurses_exports.h" is in the pre-existing "ncurses_dll.h"
• the newline-change for MKlib_gen.sh doesn't port
• some import/export declarations were overlooked (added those for building with MinGW).

So you might want to merge and see what remaining changes (such as "access") should be proposed.

[ThomasDickey]

In today's patch, I (think I) addressed these issues (except, reviewing now -- user32).

However, I see that my native-MinGW builds fail in linking the test-programs (from last week's update), so I'll be making some fixes in that area.

[madebr]

Indeed, a lot of problems are fixed in this last patch release.
I've added some patches here that allows to build/run all using MSVC, including wide characters.

• the tests also need to pass NCURSES_STATIC when building a static ncurses library
• the object file of the c++ test needs to be built in the correct folder. (Looking at my patches now, only the last patch is the correct one)
• in the tests folder, there was a place left where the build system assumes all WIN32 implementations have a flawed wchar implementation.
• The biggest patch is in c++:
  • A main function can not be imported from a dll function. That's why I renamed the main function when on _WIN32. This renaming is actually only required when building a dll.
  • static variables of a dll exported class are not exported. That's why uses of these private objects in header files would cause missing symbols because of inlining. Therefore I moved all (maybe) uses of these private variables to the .cc files. I could not add NCURSES_CXX_IMPEXP to these variables because MSVC complained with error C2487
    The biggest/weirdest change because of this reason is for titleWindow,
    This static class variable is declared protected. To avoid breakage, I added an extra function that returned this variable. This function returns a reference to a pointer. This allows one to change the pointer.

Results on CI: https://ci.appveyor.com/project/madebr/ncurses/builds/34055899
Only wide characters is failing because the alternative tsearch package for conan is not yet available.

[ThomasDickey]

I see (am reviewing the changes now).

Adding NCURSES_STATIC to libtool is a problem, since libtool is used to generate shared libraries. The normal, debug and profile libraries are static.

I found that not adding NCURSES_STATIC to the test-makefile in my native MinGW build was the problem I had from last week, so I filled out NCURSES_STATIC in the places where it seems to address your changes.

If conan doesn't have tsearch, then the ncurses library would have had an error if you had tried to use extended-colors. I don't see that in your build-logs (but there's a nuisance warning about KEY_EVENT which I may improve).

[madebr]

Adding NCURSES_STATIC to libtool is a problem, since libtool is used to generate shared libraries. The normal, debug and profile libraries are static.

I understand. Sorry for that. It would be nice if libtool had some way to divert options to shared/static builds.

I found that not adding NCURSES_STATIC to the test-makefile in my native MinGW build was the problem I had from last week, so I filled out NCURSES_STATIC in the places where it seems to address your changes.

You probably saw a lot of __imp_XXXX missing symbol errors. That's a callsign for a missing define to hide the dllimport.

If conan doesn't have tsearch, then the ncurses library would have had an error if you had tried to use extended-colors. I don't see that in your build-logs (but there's a nuisance warning about KEY_EVENT which I may improve).

Indeed, I did not pass --enable-ext-colors to the configure script. I guess it is enabled by default?
Doing --enable-widec --disable-ext-colors succeeds, but --enable-widec --enable-ext-colors fails because of the missing tsearch. I've modified my test scripts.

Anyway, only a small change to aclocal.m4 is required to make sure that LINK_TESTS is set correctly for msvc. Without it, the tests fail to link.
Logs are available at https://ci.appveyor.com/project/madebr/ncurses/builds/34182134
(ignore the combinations of widec=False and ext_colors=True)

Apart from the test fix, I don't have any further problems with building ncurses on MSVC.
Getting rid of the KEY_EVENT warning would be very nice.

Do you know of any configuration options that I may not have tested and could cause problems on MSVC?

[ThomasDickey]

I don't see anything obvious overlooked. In yesterday's update, I only saw one place (removing CFLAGS from the linker in the test-directory) which I didn't see as necessary.

[madebr]

I think the user32.lib library has not been addressed yet.

Ideally, it should be added to BUILD_LDFLAGS when built as a shared library, and added to LDFLAGS when built as a static library.

But adding it only to LDFLAGS is easier, I guess.

[ThomasDickey]

I'm puzzled regarding the BUILD_LDFLAGS vs LDFLAGS comment. BUILD_LDFLAGS is used when linking the build-time utilities (make_keys, make_hash), while LDFLAGS is used for linking the programs such as tic. Of course BUILD_LDFLAGS defaults to $LDFLAGS, mainly because autoconf isn't really designed for configuring two things at the same time.

[madebr]

I made that comment because, when creating shared libraries, only ncurses.dll should link to user32.dll.
When creating a static library, all executables linking to ncurses.lib, should also link to user32.lib.

I'm not really home in your usage of BUILD_LDFLAGS vs LDFLAGS. I hope I cleared up any confusion.

[ThomasDickey]

I'm adding a special case for user32.lib (pre-setting it before CF_BUILD_CC), and improving the KEY_EVENTS workaround. Those seem to be the unfinished issues (though it's not clear to me if workarounds such as tsearch are part of your current builds).

[madebr]

... it's not clear to me if workarounds such as tsearch are part of your current builds).

The conanfile contains the complete build script.
Apart from the tseach workaround, the only thing that attracts my eyes are the following lines:

        if self.settings.os == "Windows":
            self._autotools.defines.append("_WIN32")
            if self.settings.arch == "x86_64":
                self._autotools.defines.append("_WIN64")

I'm not sure whether these are really needed.
I have copied them from an earlier conan build script, and didn't check further.
Once I've started my Windows box, I'll let you know.

[madebr]

appveyor ci of this pr should become available at https://ci.appveyor.com/project/madebr/ncurses/builds/34668185

• While adding the Visual Studio patches to conan at ncurses: add shared + program support for MSVC conan-io/conan-center-index#2351,
  the CI error'ed when using Visual Studio 2019 and MT (=static non-debug) runtime .
  I had to modify the mk_prog.sh script to strip out -MT,-MD,-MTd and -MDd.

• I could remove _WIN32 and _WIN64, without change of behavior. I also removed it from the conan recipe.
  So it will be tested on all MSVC relevant configurations from 2015 until 2019.

[madebr]

I just wanted to point out that all jobs on appveyor are now succeeding as Conan now has a tsearch implementation available.

[ThomasDickey]

sounds good.

The case statement would be simpler as
-M[TD] | -M[TD]d)

I did that before digging into Juergen's changes. Those will take a few days of development before they're ready (perhaps by next weekend).

[madebr]

Thanks for helping me upstream these patches.
I'll close this pr as msvc is now supported.

[ThomasDickey]

sounds good - you can make a new pull request to handle regressions or new features