patchgnokii - Patches: patch #8934, Restore WIn32 (cross-) compiling

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #8934: Restore WIn32 (cross-) compiling

Submitter:  Fabrizio Gennari <fabbo>
Submitted:  Sun 06 Mar 2016 10:08:31 PM UTC
   
 
Category:  win32 compability Priority:  5 - Normal
Status:  Ready For Test Privacy:  Public
Assigned to:  ladis Open/Closed:  Open

Jump to the original submission

Sun 26 Dec 2021 09:37:55 PM UTC, comment #10: 

comment #9:

> Fabrizio,
>
> please check https://github.com/3x380V/gnokii/tree/preview3 to see whenever it works for you. Thanks!


Hi, it took more than 1 year, but eventually I tested the branch preview4, and it works on Windows. Thank you.

One note: it requires ac_func_mkdir.m4 from Autoconf Macro Archive (http://ac-archive.sourceforge.net/ac-archive/ac_func_mkdir.html). On one hand, I think it should be added to m4. On the other hand, commit bd3f583b41a7f7a3e7add3d6c1f370d012a15ec7 on that branch reads "Remove m4/*.m4 files which are present in autoconf-archive. Remember to add autoconf-archive into build dependencies", so apparently copying those files manually (or "adding autoconf-archive into build dependencies", whatever it means) is the way to do it.

Fabrizio Gennari <fabbo>
Sat 21 Nov 2020 08:12:37 PM UTC, comment #9: 

Fabrizio,

please check https://github.com/3x380V/gnokii/tree/preview3 to see whenever it works for you. Thanks!

Ladislav Michl <ladis>
Group Member
Mon 03 Dec 2018 09:16:40 AM UTC, comment #8: 

Fabrizio,

  • (0005) It is not libgnokii problem some project using it also needs c99 compatible snprintf. Gnokii should link snprintf.c on its own (and I left it as an excercise of a reader ;-))
  • (0001) That's quite nice explanation, indeed. Now, could it be part of commit message? In somewhat condensed form.
  • (0007) This is quite the opposite! There's no point invoking compiler on a file which is no-op when we already know it is no-op. So leaving Makefile as it is and removing #ifdefs from source is a way to go. I tried to implement more finegrained approchach, patch on ml.


I'll send updated [*] patchset to the mailinglist (as I didn't find this savannah patch system too convenient). Please review there and add what's still missing to you.

[*] Here updated means:

  • 0006 was replaced with AC_FUNC_MKDIR
  • 0004 introduced problem when we also need gettimeofday, so replaced
  • 0003 version with posix_spawn introduced


Please note, that I'm not able to test Win32 changes currently - glib headers itself fails to compile in current debian unstable. Hopefully I'll find some time next week to look at it.

Ladislav Michl <ladis>
Group Member
Tue 27 Nov 2018 09:10:48 PM UTC, comment #7: 

Small addition: removing GNOKII_API from the definitions of rep_fprintf and rep_snprintf causes 308 lines of errors from the linker. Below the first 8, that are representative of the others


/bin/bash ../libtool   --tag=CC   --mode=link i686-w64-mingw32-gcc -I../include -g -O2 -Wall -Wno-pointer-sign -fvisibility=hidden -fno-strict-aliasing    -o gnokii.exe gnokii-gnokii.o gnokii-gnokii-calendar.o gnokii-gnokii-dial.o gnokii-gnokii-file.o gnokii-gnokii-logo.o gnokii-gnokii-mms.o gnokii-gnokii-monitor.o gnokii-gnokii-other.o gnokii-gnokii-phonebook.o gnokii-gnokii-profile.o gnokii-gnokii-ringtone.o gnokii-gnokii-security.o gnokii-gnokii-settings.o gnokii-gnokii-sms.o gnokii-gnokii-todo.o gnokii-gnokii-utils.o gnokii-gnokii-wap.o ../common/libgnokii.la    ../getopt/libgetopt.a -lwinmm -lws2_32
libtool: link: i686-w64-mingw32-gcc -I../include -g -O2 -Wall -Wno-pointer-sign -fvisibility=hidden -fno-strict-aliasing -o .libs/gnokii.exe gnokii-gnokii.o gnokii-gnokii-calendar.o gnokii-gnokii-dial.o gnokii-gnokii-file.o gnokii-gnokii-logo.o gnokii-gnokii-mms.o gnokii-gnokii-monitor.o gnokii-gnokii-other.o gnokii-gnokii-phonebook.o gnokii-gnokii-profile.o gnokii-gnokii-ringtone.o gnokii-gnokii-security.o gnokii-gnokii-settings.o gnokii-gnokii-sms.o gnokii-gnokii-todo.o gnokii-gnokii-utils.o gnokii-gnokii-wap.o  ../common/.libs/libgnokii.dll.a -L/home/fabrizio/dev/gnokii/glibwin/lib -lglib-2.0 -lintl ../getopt/libgetopt.a -lwinmm -lws2_32 -L/usr/local/lib
/usr/bin/i686-w64-mingw32-ld: gnokii-gnokii.o: in function `usage':
/home/fabrizio/dev/gnokii/gnokii/gnokii.c:225: undefined reference to `rep_fprintf'
/usr/bin/i686-w64-mingw32-ld: gnokii-gnokii.o: in function `install_log_handler':
/home/fabrizio/dev/gnokii/gnokii/gnokii.c:343: undefined reference to `rep_snprintf'
/usr/bin/i686-w64-mingw32-ld: /home/fabrizio/dev/gnokii/gnokii/gnokii.c:323: undefined reference to `rep_snprintf'
/usr/bin/i686-w64-mingw32-ld: /home/fabrizio/dev/gnokii/gnokii/gnokii.c:346: undefined reference to `rep_fprintf'


Fabrizio Gennari <fabbo>
Tue 27 Nov 2018 08:55:31 PM UTC, comment #6: 


> * 0005-update-snprintf.c-replacement-of-snprintf-for-system.patch: why are
> some printf family functions marked with GNOKII_API? If any libgnokii app
> needs them, it should use snprintf.c directly.

But then the same problem that led to adding snprintf.c to the source tree will come back. snprintf.c is there to provide a C99-compatible implementation of the printf family of functions. Both libgnokii and the main gnokii app need such functions: if they were not exported by libgnokii, libgnokii would have access to the C99-compatible printf functions, but the gnokii app would need to use the non-C99-compatible printf functions provided by Windows.

>
>
> * 0001-Use-the-native-Autoconf-support-for-cross-compilatio.patch:
> AM_CONDITIONAL FOR_MAC changed. Why? This is kind of change commit log should
> explain. Otherwise change is worth testing and applying.

The only thing FOR_MAC practically changes is whether the file osxbluetooth.m should be built. In the previous version, an additional check was done, whether Bluetooth support was actually detected by Autoconf. That is not necessary, because, if that support is not there, HAVE_BLUETOOTH_MACOSX is false and the whole file osxbluetooth.m becomes empty, so linking it or not linking it makes no difference. The naming of FOR_MAC is actually best suited to the new approach.

>
> * 0007-include-in-Win32-build-some-stuff-that-does-not-need.patch : there's
> FIXME deleted, but patch itself does not explain what changed to make this
> fixed. Also now it links in tcp.c for WIN32 but whole file is #ifndef WIN32
> protected. What justifies this change?


The point is that #ifndef WIN32 is supposed to exclude code that's not expected to compile or work on Windows, but some of that code actually compiles and works on Windows, so such #ifndef WIN32 have been removed. No changes have been necessary to make that code work.

Also, the very fact that tcp.c is #ifndef WIN32 protected makes an exception in the Makefile unnecessary. The same is true for socketphonet.c, which is protected by #ifndef HAVE_SOCKETPHONET. No need to exclude those files both in the Makefile and in code with #ifdef's, the latter alone is enough.

Fabrizio Gennari <fabbo>
Wed 21 Nov 2018 10:09:34 AM UTC, comment #5: 

Fabrizio, thank you for an update, now it is definitely much easier to review.

All patches need commit message fixed. Well, perhaps it is not really necessary once someone is willing to fix that while applying. To save maintainers job patches should be ready to process with 'git am'.

Now one by one:

  • 0005-update-snprintf.c-replacement-of-snprintf-for-system.patch: why are some printf family functions marked with GNOKII_API? If any libgnokii app needs them, it should use snprintf.c directly.
  • 0006-Since-in-Windows-mkdir-takes-1-argument-a-Autoconf-t.patch: Ok
  • 0001-Use-the-native-Autoconf-support-for-cross-compilatio.patch: AM_CONDITIONAL FOR_MAC changed. Why? This is kind of change commit log should explain. Otherwise change is worth testing and applying.
  • 0007-include-in-Win32-build-some-stuff-that-does-not-need.patch : there's FIXME deleted, but patch itself does not explain what changed to make this fixed. Also now it links in tcp.c for WIN32 but whole file is #ifndef WIN32 protected. What justifies this change?
  • 0002-Since-Linux-filesystems-are-case-sensitive-and-MinGW.patch: ok
  • 0003-Add-an-autoconf-test-for-fork-to-support-systems-tha.patch : an alternative approach is about to be send to the mailing list


Ladislav Michl <ladis>
Group Member
Mon 12 Nov 2018 08:54:40 PM UTC, comment #4: 

Well, you even added support for completely new toolchains (MSVC2015), changed includes case and removed ifdefs in one go. For configurations you have tried that works. But try to answer yourself a questions:
- How does changing includes case affects MSVC2015?
- How could you know all those removed WIN32 ifdefs do not affect configurations you did not try?
- How do you know your configure change won't break *BSD build?
etc...

And now imagine situation someone fetches your update after long time and report problem. Your interest in this project is lost and the only sane option is reverting whole commit. Unless, of course, someone other invest time and fixes it proper way. But we cannot count on this and therefore splitting patch so it does one logical change at time is the only viable option.

Yes, I know, this is really annoying, but with a little practice easy to do.

Ladislav Michl <ladis>
Group Member
Mon 12 Nov 2018 08:05:26 PM UTC, comment #3: 

Also, about splitting the patch, I have doubts. The whole patch turns a broken build into a working build, and there's no such thing as a "half-broken" build: either it's broken or it's not. It's not in my opinion worth giving individual patches, each of which making the build "less broken".

I can, however, be open to splitting off the sprintf part of the patch, at least for its sheer size (it takes 1600 lines on a total of 2500 for the whole patch). Let me know if that solution is worth considering.

Fabrizio Gennari <fabbo>
Sat 10 Nov 2018 08:08:59 PM UTC, comment #2: 

Sorry for the misunderstanding: the second patch (file #36699) actually replaces the first one, it is not supposed to be applied after the first one. The fact is that the first patch fixes MSYS2 compilation and Linux cross-compilation, then, a few days after, I also fixed Visual Studio compilation and posted a single patch that fixed everything.

Fabrizio Gennari <fabbo>
Sat 10 Nov 2018 07:57:36 PM UTC, comment #1: 

Those two patches do not apply one after other. I suggest splitting them, so each do just one thing: snprintf update, case-sensitive aware includes, configure update, etc...

Ladislav Michl <ladis>
Group Member
Sun 06 Mar 2016 10:08:31 PM UTC, original submission:  

This patch restores Win32 compilation under GNU shell environments. Tested: Linux cross-compiling (Ubuntu) and compiling on Windows using MSYS2.

Summary:

  • common/compat.c wasn't compiling because on Windows both #ifndef HAVE_SETENV and #ifdef WIN32 are true
  • common/device.c wasn't compiling because of fork() and waitpid(). Added an autoconf test for fork() to avoid that
  • In directory common/devices, compile empty stubs for socketphonet.c and tcp.c
  • Since Linux filesystems are case-sensitive, and MinGW-W64 provides lower case names for Win32 header files, use lower case names in #include directives
  • common/snprintf.c, which was originally taken from Samba, has been updated to the latest version from the Samba Git repository (https://git.samba.org/?p=samba.git;a=blob;f=lib/replace/snprintf.c;hb=5f08d8b1728cc4559e61e0c928577724e0e2f9d2)
  • In configure.ac, option --enable-win has been removed. Instead, the standard Autoconf way to cross-compile is used, that is using option --host. For example, if the cross-tools have names prefixed by i686-w64-mingw32- (i686-w64-mingw32-gcc, i686-w64-mingw32-pkg-config etc.), pass --host=i686-w64-mingw32 .
  • As an indirect consequence, in order to detect when compiling for Linux, *BSD or Mac, uname (which returns the system configure is running on, which might not be the same system for which the tool is built) is not used, instead, the Autoconf-set variable $host_os is used. This allows in theory cross-compilation between Linux, *BSD and Mac.

- Since in Windows mkdir() takes 1 argument, a Autoconf test for the number of arguments in mkdir() and a replacement mkdir() have been provided. Both are inspired from what Samba does.

  • a couple of unnecessary #ifndef WIN32 have been removed
Fabrizio Gennari <fabbo>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #45466:  0005-update-snprintf.c-replacement-of-snprintf-for-system.patch added by fabbo (46KiB - text/x-patch - Broken-down patch set (5-7 of 7))
file #45467:  0006-Since-in-Windows-mkdir-takes-1-argument-a-Autoconf-t.patch added by fabbo (2KiB - text/x-patch - Broken-down patch set (5-7 of 7))
file #45468:  0007-include-in-Win32-build-some-stuff-that-does-not-need.patch added by fabbo (3KiB - text/x-patch - Broken-down patch set (5-7 of 7))
file #45462:  0001-Use-the-native-Autoconf-support-for-cross-compilatio.patch added by fabbo (5KiB - text/x-patch - Broken-down patch set (1-4 of 7))
file #45463:  0002-Since-Linux-filesystems-are-case-sensitive-and-MinGW.patch added by fabbo (2KiB - text/x-patch - Broken-down patch set (1-4 of 7))
file #45464:  0003-Add-an-autoconf-test-for-fork-to-support-systems-tha.patch added by fabbo (3KiB - text/x-patch - Broken-down patch set (1-4 of 7))
file #45465:  0004-On-Windows-both-ifndef-HAVE_SETENV-and-ifdef-WIN32-a.patch added by fabbo (1KiB - text/x-patch - Broken-down patch set (1-4 of 7))
file #36699:  0001-Restore-Win32-native-compilation-with-MSYS2-and-Visu.patch added by fabbo (78KiB - text/x-patch - Updated patch with Visual Studio fixes and Visual studio 2015 solution and project files)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by ladis (Posted a comment)
  • -email is unavailable- added by fabbo (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-11-21 ladis StatusIn Progress Ready For Test
    2018-11-17 fabbo Attached File- Added 0005-update-snprintf.c-replacement-of-snprintf-for-system.patch, #45466
        Attached File- Added 0006-Since-in-Windows-mkdir-takes-1-argument-a-Autoconf-t.patch, #45467
        Attached File- Added 0007-include-in-Win32-build-some-stuff-that-does-not-need.patch, #45468
    2018-11-17 fabbo Attached File- Added 0001-Use-the-native-Autoconf-support-for-cross-compilatio.patch, #45462
        Attached File- Added 0002-Since-Linux-filesystems-are-case-sensitive-and-MinGW.patch, #45463
        Attached File- Added 0003-Add-an-autoconf-test-for-fork-to-support-systems-tha.patch, #45464
        Attached File- Added 0004-On-Windows-both-ifndef-HAVE_SETENV-and-ifdef-WIN32-a.patch, #45465
    2018-11-10 ladis StatusNone In Progress
        Assigned toNone ladis
    2016-03-20 fabbo Attached File- Added 0001-Restore-Win32-native-compilation-with-MSYS2-and-Visu.patch, #36699
    2016-03-06 fabbo Attached File- Added 0001-Restore-Win32-cross-compilation-from-Linux.patch, #36560

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code