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

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

( Jump to the original submission)

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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>

 

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)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

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