Hugin can be built with MinGW now!

[Jan Dubiec]

So finally I have managed to build the latest Hugin's default branch
with MinGW! There are only minor changes in the source code but I have
spent a lot of time in order to find out what to change. :-) I have
split the patch into three parts:
1. source_files.diff - modifies 10 C++ source/header files,
2. cmake_files.diff - modifies 9 CMake files,
3. nsis_files.diff - modifies 2 NSIS files.

After applying these changes I have successfully built Hugin with MSVC++
2010 Express and MinGW-W64 5.1.0, however I have focused only on shared
Hugin i.e -DHUGIN_SHARED=1 on CMake's command line. In case of MinGW I
have used "MSYS Makefiles" generator.

Please let me know if you have any questions.

Jan

[T. Modes]

Thanks for your work.
But the splitting makes it not easier to comment on it. The source and CMake changes are partially related and need to keep together.
On the other hand the patches contains many unrelated changes: from necessary changes, over warning fixes to cosmetic changes.
Please break it into smaller chunks which addresses only one issue at one time. This makes it easier to comment on it.

Just some comments:
* The changes break the installation of wxWidgets in MSVC version. You did probably test with a existing build and not from a clean one.
* Only the code in src\hugin1 depends on wxWidgets. But you pull in wxWidgets header also in hugin_base. Hugin_base should be free of dependencies on wxWidgets (This is a design decision.)
* Usage of empty ${wxWidgets_CONFIG_OPTIONS} variable.
* Test of HAVE_LOG1P without pulling in the config file where it is defined.
* There are also gcc versions which use a static runtime by default, or which don't support OpenMP or which are using the win32 threading model. So don't install unconditional all dll.
* Install target for lapack is also using gcc files also for MSVC:

IF(NOT MINGW)
   INSTALL(FILES ${LIBGCC_DLL}...
* Where are USE_JPEG_DLL, ... used? I don't see them in wxWidgets or libjpeg.

So in the current form the patch can't be applied.

[Jan Dubiec]

On 2015-09-27 09:01, T. Modes wrote:
[...]

> Please break it into smaller chunks which addresses only one issue at
> one time. This makes it easier to comment on it.

OK. In the attachment there is a bunch of smaller, "cosmetic" patches.
More "serious" stuff will come later.

> Just some comments:
> * The changes break the installation of wxWidgets in MSVC version. You
> did probably test with a existing build and not from a clean one.

Yes, I use "blessed" version for MSVC++ 2010 from
http://sourceforge.net/projects/wxwindows/files/3.0.2/binaries/. They
have vc100 suffix.

> * Only the code in src\hugin1 depends on wxWidgets. But you pull in
> wxWidgets header also in hugin_base. Hugin_base should be free of
> dependencies on wxWidgets (This is a design decision.)

OK, I didn't know about it. On the other hand someone else added
wxWingets related stuff to hugin_base/hugin_utils/utils.h
(wxLogWarning, wxLogError, wxLogFatalError, wxString and wxConvISO8859_1).

> * Usage of empty ${wxWidgets_CONFIG_OPTIONS} variable.

Where? Anyway, variable wxWidgets_CONFIG_OPTIONS contains options which
are passed to wx-config utility, it may be empty and by default it is
empty. See FindwxWidgets.cmake for more information.

> * Test of HAVE_LOG1P without pulling in the config file where it is defined.

Hmm, I don't understand because I also have changed
src/hugin_config.h.in.cmake. Anyway, I will make separate patch for
this. UPDATE: I've just seen that you did it. :-)

> * There are also gcc versions which use a static runtime by default, or
> which don't support OpenMP or which are using the win32 threading model.
> So don't install unconditional all dll.

OK. I assumed MinGW-W64 as it seems to be the most popular version. This
issue may be discussed later. The most important thing is to make the
sources compilable with MinGW.

> * Install target for lapack is also using gcc files also for MSVC:
>
> IF(NOT MINGW) INSTALL(FILES ${LIBGCC_DLL}...

Yes, because I use LAPACK built by myself which dynamically links to
libgcc, libgfortran, etc. just like the "official" one:
http://icl.cs.utk.edu/lapack-for-windows/lapack/. However IMO it's not a
big deal - I am quite sure that it is possible to build LAPACK's DLLs
with libgcc and libgfortran statically linked.

> * Where are USE_JPEG_DLL, ... used? I don't see them in wxWidgets or
> libjpeg.

In jmorecfg.h from jpeg-9a, line 245. Jmorecfg.h is included by jpeglib.h.

Jan

[T. Modes]

Am Sonntag, 27. September 2015 15:13:19 UTC+2 schrieb jdx:

> * The changes break the installation of wxWidgets in MSVC version. You
> did probably test with a existing build and not from a clean one.
Yes, I use "blessed" version for MSVC++ 2010 from
http://sourceforge.net/projects/wxwindows/files/3.0.2/binaries/. They
have vc100 suffix.

But your code will then only work with the 32 bit version, the 64 bit version does not work with your code.
Until now we have used a self compiled wxWidgets library, and for this use case the filename work as there.

 

> * Only the code in src\hugin1 depends on wxWidgets. But you pull in
> wxWidgets header also in hugin_base. Hugin_base should be free of
> dependencies on wxWidgets (This is a design decision.)
OK, I didn't know about it. On the other hand someone else added
wxWingets related stuff to hugin_base/hugin_utils/utils.h
(wxLogWarning, wxLogError, wxLogFatalError, wxString and wxConvISO8859_1).

Ohh, that's an oversight. I removed the wxStuff from hugin_base

 

> * Usage of empty ${wxWidgets_CONFIG_OPTIONS} variable.
Where? Anyway, variable wxWidgets_CONFIG_OPTIONS contains options which
are passed to wx-config utility, it may be empty and by default it is
empty. See FindwxWidgets.cmake for more information.

But we don't set it currently. So it remains empty and adding it makes all more complicated than it is already. So we can currently ignore it.

 

> * Install target for lapack is also using gcc files also for MSVC:
>
> IF(NOT MINGW) INSTALL(FILES ${LIBGCC_DLL}...
Yes, because I use LAPACK built by myself which dynamically links to
libgcc, libgfortran, etc. just like the "official" one:
http://icl.cs.utk.edu/lapack-for-windows/lapack/. However IMO it's not a
big deal - I am quite sure that it is possible to build LAPACK's DLLs
with libgcc and libgfortran statically linked.

But even when using the prebuilt version you can't pull in files from gcc (because it is not installed when using MSVC compiler, there are then only the dll).
LAPACK can be ignored, the code which is improved when compiling with LAPACK, is not used by Hugin.
So these dependencies can be ignored. Maybe we should remove the LAPACK stuff from Hugins Cmake system.
 

> * Where are USE_JPEG_DLL, ... used? I don't see them in wxWidgets or
> libjpeg.
In jmorecfg.h from jpeg-9a, line 245. Jmorecfg.h is included by jpeglib.h.

 No. In official jpeg-9a, file jmorecfg.h line 245 reads:
/* a function referenced thru EXTERNs: */
#define GLOBAL(type)        type

So your are using a patched version.

And now to your patches:
* nominmax.diff: is purely cosmetic. So I will postpone this one.
* findcmakemodules.diff: the glew part is okay, but the tiff part not. With MSVC libtiff_i.lib is the dynamic version and libtiff.lib the static one. When searching for both names in the same line, this can pull in the static version in the dynamic build scenario (as already stated in the comment directly above). This I want avoid and can therefore not search for both in your way. I committed a slightly modified version.

The other one I committed in current form.

[Jan Dubiec]

On 2015-09-27 16:47, T. Modes wrote:
>
>
> Am Sonntag, 27. September 2015 15:13:19 UTC+2 schrieb jdx:
>
> > * The changes break the installation of wxWidgets in MSVC version.
> You
> > did probably test with a existing build and not from a clean one.
> Yes, I use "blessed" version for MSVC++ 2010 from
> http://sourceforge.net/projects/wxwindows/files/3.0.2/binaries/

> <http://sourceforge.net/projects/wxwindows/files/3.0.2/binaries/>. They

> have vc100 suffix.
>
>
> But your code will then only work with the 32 bit version, the 64 bit
> version does not work with your code.
> Until now we have used a self compiled wxWidgets library, and for this
> use case the filename work as there.

OK, this needs more work. But does the original code work properly with
32- and 64-bit libraries? Because suffix "custom" means nothing.

> > * Usage of empty ${wxWidgets_CONFIG_OPTIONS} variable.
> Where? Anyway, variable wxWidgets_CONFIG_OPTIONS contains options which
> are passed to wx-config utility, it may be empty and by default it is
> empty. See FindwxWidgets.cmake for more information.
>
>
> But we don't set it currently. So it remains empty and adding it makes
> all more complicated than it is already. So we can currently ignore it.

For example I set this variable at CMake's command line. It is useful if
you have multiple wxWidgets configurations (eg. "debug" and "release")
or if your actual wxWidgets prefix is other that the one specified at
configure stage.

> > * Install target for lapack is also using gcc files also for MSVC:
> >
> > IF(NOT MINGW) INSTALL(FILES ${LIBGCC_DLL}...
> Yes, because I use LAPACK built by myself which dynamically links to
> libgcc, libgfortran, etc. just like the "official" one:
> http://icl.cs.utk.edu/lapack-for-windows/lapack/

> <http://icl.cs.utk.edu/lapack-for-windows/lapack/>. However IMO it's

> not a
> big deal - I am quite sure that it is possible to build LAPACK's DLLs
> with libgcc and libgfortran statically linked.
>
>
> But even when using the prebuilt version you can't pull in files from
> gcc (because it is not installed when using MSVC compiler, there are
> then only the dll).

In fact gcc is installed. :-) But I get your point. The proper way is to
distribute libgfortran.dll & Co. with LAPACK's DLLs or link it
statically into them.

> LAPACK can be ignored, the code which is improved when compiling with
> LAPACK, is not used by Hugin.
> So these dependencies can be ignored. Maybe we should remove the LAPACK
> stuff from Hugins Cmake system.

Hmmm... I build Hugin with -DENABLE_LAPACK=ON and Dependency Walker
shows me that libhuginbase.dll depends on liblapack.dll.

> No. In official jpeg-9a, file jmorecfg.h line 245 reads:
> /* a function referenced thru EXTERNs: */
> #define GLOBAL(type) type
>
> So your are using a patched version.

Indeed I am. :-) For some reason I added to jmorecfg.h usual
__declspec(dllexport/dllimport) stuff.

> With MSVC libtiff_i.lib is the dynamic version and libtiff.lib the
> static one. When searching for both names in the same line, this can
> pull in the static version in the dynamic build scenario (as already
> stated in the comment directly above). This I want avoid and can
> therefore not search for both in your way. I committed a slightly
> modified version.

Oh, there is my fault to some extent because I have libtiff with
non-standard file name. By default, when built with MinGW, libtiff's
4.0.4 DLL is called libtiff-5.dll (five, not four) and I have changed it
to simple libtiff.dll. Static library is called libtiff.a and import
library is libtiff.dll.a.


Three new patches in the attachment. CMakeLists.diff removes unnecessary
wxWidgets related definitions at src directory level and two other
patches are critical - without them compilation and/or linking fails due
to some symbols redefinition.

Jan

[T. Modes]

Am Mittwoch, 30. September 2015 15:39:51 UTC+2 schrieb jdx:

> But your code will then only work with the 32 bit version, the 64 bit
> version does not work with your code.
> Until now we have used a self compiled wxWidgets library, and for this
> use case the filename work as there.
OK, this needs more work. But does the original code work properly with
32- and 64-bit libraries? Because suffix "custom" means nothing.

But it is the default suffix which is used when self-compiling wxWidgets.

 

>     > * Usage of empty ${wxWidgets_CONFIG_OPTIONS} variable.
>     Where? Anyway, variable wxWidgets_CONFIG_OPTIONS contains options which
>     are passed to wx-config utility, it may be empty and by default it is
>     empty. See FindwxWidgets.cmake for more information.
>
>
> But we don't set it currently. So it remains empty and adding it makes
> all more complicated than it is already. So we can currently ignore it.
For example I set this variable at CMake's command line. It is useful if
you have multiple wxWidgets configurations (eg. "debug" and "release")
or if your actual wxWidgets prefix is other that the one specified at
configure stage.

Then you are the first which needs it. The current code is used several years also on *nix system and nobody complained about it.

 

>     > * Install target for lapack is also using gcc files also for MSVC:
>

> But even when using the prebuilt version you can't pull in files from
> gcc (because it is not installed when using MSVC compiler, there are
> then only the dll).
In fact gcc is installed. :-)

But not when compiling with MSVC.
 

Hmmm... I build Hugin with -DENABLE_LAPACK=ON and Dependency Walker
shows me that libhuginbase.dll depends on liblapack.dll.

But that does not mean that the code is actually used by Hugin.
It can be used by other programs with are using levmar.

Oh, there is my fault to some extent because I have libtiff with
non-standard file name. By default, when built with MinGW, libtiff's
4.0.4 DLL is called libtiff-5.dll (five, not four) and I have changed it
to simple libtiff.dll. Static library is called libtiff.a and import
library is libtiff.dll.a.

??? Who had this idea..

Three new patches in the attachment. CMakeLists.diff removes unnecessary
wxWidgets related definitions at src directory level and two other
patches are critical - without them compilation and/or linking fails due
to some symbols redefinition.

I committed your patches.