Exiv2

Are you sure these aren't needed (both the headers and the linker statements)?

Yes. (I removed them in my local build and it still compiles and works. I removed the includes and pragmas from config.h, only #include <windows.h> was needed in rwlock.hpp and #pragma comment(lib, "ws2_32.lib") in http.cpp, nothing in basicio.cpp)
In version 0.25 these includes were not in config.h and it worked fine. And in 0.26 the API has not changed as far as I can see. So there are no additional requirements in the public headers which would require the additional headers.

All exiv2 applications can perform IO with class HttpIo which requires winsock.

But only exiv2.dll requires winsock.lib, not the calling program - because all call to the http are capsuled into the dynamic library. The calling program does not know about these internal and therefore don't need it.

After removing the pragma and compiling with MSVC 2015 I get the following dependency charts with Dependency Walker:

[  6] EXIV2.EXE
     [  6] KERNEL32.DLL
     [  6] EXIV2.DLL  <---
          [ ^6] KERNEL32.DLL
          [  6] SHELL32.DLL
          [  6] ZLIB.DLL
          [ ^6] MSVCP140.DLL
          [ ^6] WS2_32.DLL         <---
          [  6] PSAPI.DLL
          [ ^6] VCRUNTIME140.DLL
          [ ^6] API-MS-WIN-CRT-RUNTIME-L1-1-0.DLL
          [ ^6] API-MS-WIN-CRT-STDIO-L1-1-0.DLL
          [ ^6] API-MS-WIN-CRT-FILESYSTEM-L1-1-0.DLL
          [ ^6] API-MS-WIN-CRT-HEAP-L1-1-0.DLL
          [ ^6] API-MS-WIN-CRT-CONVERT-L1-1-0.DLL
          [ ^6] API-MS-WIN-CRT-TIME-L1-1-0.DLL
          [ ^6] API-MS-WIN-CRT-MATH-L1-1-0.DLL
          [ ^6] API-MS-WIN-CRT-ENVIRONMENT-L1-1-0.DLL
          [ ^6] API-MS-WIN-CRT-STRING-L1-1-0.DLL
          [  6] EXPAT.DLL
     [  6] MSVCP140.DLL
     [  6] VCRUNTIME140.DLL
     [  6] API-MS-WIN-CRT-RUNTIME-L1-1-0.DLL
     [  6] API-MS-WIN-CRT-STDIO-L1-1-0.DLL
     [  6] API-MS-WIN-CRT-FILESYSTEM-L1-1-0.DLL
     [  6] API-MS-WIN-CRT-HEAP-L1-1-0.DLL
     [  6] API-MS-WIN-CRT-STRING-L1-1-0.DLL
     [  6] API-MS-WIN-CRT-CONVERT-L1-1-0.DLL
     [  6] API-MS-WIN-CRT-TIME-L1-1-0.DLL
     [  6] API-MS-WIN-CRT-ENVIRONMENT-L1-1-0.DLL
     [  6] API-MS-WIN-CRT-MATH-L1-1-0.DLL
     [  6] API-MS-WIN-CRT-LOCALE-L1-1-0.DLL

I have a couple of questions:

1) What harm is this causing?

It breaks the compilation of programs using libexiv2 (Hugin in my case, maybe also other programs).
It works fine with exiv2 0.25. (And I see no further changes in the normal headers exif.hpp, image.hpp and so on, only config.h changed).
Even it I could fix the compilation issues it would pull in these additional libraries, which I don't need and want.

2) Why does this need to be changed?

See point 1)
This way you are also mixing config variables and program logic (which types and functions are needed from other libs).
IMHO the public header should only include these headers which are needed in the public interface. Winsock2.h and the other one are not needed in the public interface.

T:

Fix submitted: r4761

I have retained the #include <windows.h> in config.h. That's required by rwlock.hpp and I've moved the pragma to link winsock into http.cpp. class HttpIo in basicio.cpp uses http.cpp to perform the socket io. So basicio.cpp does not use winsock and is unchanged.

Can you review these changes in the Hugin build and confirm that we are closed on this matter.

I think I can remove the #include <windows.h> if I do some refactoring of rwlock.hpp However if you are happy with the fix in r4761, I'd prefer not to touch rwlock.hpp as I didn't write that code.

I'm hoping to publish the v0.26 release in the next few days. I won't release it until you've had a chance to look r4761.

Robin

Hi Robin,

thanks for the fix. It is a step forward. I'm getting less errors.
If the <windows.h> is only needed by rwlook.hpp, it should be moved into that file.

In rwlock.hpp we have already

#ifndef _MSC_VER
#include <pthread.h>
#endif

changing this to

ifdef _MSC_VER
#include <windows.h>
#else
#include <pthread.h>
#endif

This would be in accordance with the code below which is also divided into 3 sections.
In my test it works fine and fixes my problems. (I can remove #include <windows.h> from config.h)

T:

Not so fast. rwlock.hpp has inline code that requires <windows.h>. rwlock is needed by properties.hpp which is included in include/exiv2/exiv2.hpp. So <windows.h> will be pulled in by the public headers. The fix is to refactor rwlock.hpp into rwlock.hpp and rwlock.cpp and <windows.h> is only required in rwlock.cpp This can be done of course - it's only work and I wanted to get your feedback before doing that.

I'll get on with this. It shouldn't take long (we know that's not true, a new file will break something in the build).

Robin

Robin,

include/exiv2/exiv2.hpp pulls also now <windows.h> via config.h -> futils.hpp/types.hpp -> include/exiv2/exiv2.hpp.
So my proposal would only move down nearer to the place where needed.
So moving down the include does not affect the public headers (no need for new files), but would fixes the error for me.

For explanation: Hugin does not include <exiv2/exiv2.hpp>, it includes only the necessary headers exif.hpp, easyaccess.hpp and image.hpp, which pull now into <windows.h> via types.hpp -> config.h.

Ah. My recommended way to use exiv2 is:

#include <exiv2/exiv2.hpp>

I've moved include <windows.h> into rwlock.hpp:

...
#ifndef RW_LOCK_HPP
#define RW_LOCK_HPP

#ifdef   _MSC_VER
#include <windows.h>
#else
#include <pthread.h>
#endif

namespace Exiv2 {
...

Can you send me a patch of what is working for you?

// Here's a note that I prepared earlier after investigating refactoring rwlock.hpp.

I've had a look into this. Not so easy. There's a static RWLock in properties.hpp. class RWLock is platform dependent. On MSVC <= 2012, it has a private CRITICAL_SECTION, else an SRWLOCK. So rwlock.hpp still needs <windows.h> for those private members of class RWLock. The work-around is to declare RWLock with a private member char lock[80] and memcpy a CRITICAL_SECTION/SRWLOCK into lock. Loads of casts and stuff, however the only code that would really know what's in lock is rwlock.cpp This would be a mess who's only purpose is to avoid having <windows.h> included by include/exiv2/exiv2.hpp I don't feel it's a pain to include <windows.h> when building with MSVC.

Incidentally, a user sent me the rwlock.hpp code. I do not like locks. Multi-threading is subtle and locks are seldom a good idea. However, I try to give everybody what they want and I accepted the rwlock.hpp code.

r4764

I reverted the changes concerning windows.h in r4763 because they broke the cygwin build. However I've retained the changes in exiv2/exiv2.hpp and exv_msvc.h as they are important/valid changes. I don't expect the changes to exiv2/exiv2.hpp and exv_msvc.h to impact Hugel.

At least for rwlock.hpp I have used #ifdef _MSC_VER intentionally (and not defined (__WIN32) or ... defined(__CYGWIN__) || defined(__MINGW__)). The code in rwlock.hpp provide 3 different implementations: 2 for MS Visual Studio compilers of different ages, these needs <windows.h> for implementation because of the used functions. All other compiler (so cygwin, MinGW, gcc, and so on) use an implementation for class RWLock based on <pthread.h> (see also line 145 in rwlock.hpp). This implementation does not need <windows.h> and so the inclusion of defined (CYGWIN) and defined(MINGW) was not needed in this file (and not included in my patch).

T:

So where are we now? Do you have a build issue when you build Hugin with r4764?

Your analysis is correct. rwlock.hpp has three platform dependent implementations.

535 rmills@rmillsmbp:~/gnu/exiv2/trunk $ grep -e '#' include/exiv2/rwlock.hpp 
#ifndef RW_LOCK_HPP
#define RW_LOCK_HPP
#ifdef _MSC_VER
#include <windows.h>
#else
#include <pthread.h>
#endif
#ifdef _MSC_VER
#if _MSC_VER >= 1800
#else
#if defined(MSDEV_2003) || defined(MSDEV_2005)
#else
#endif
#endif
#else
#endif
#endif // RW_LOCK_HPP
536 rmills@rmillsmbp:~/gnu/exiv2/trunk $ 

I'm sure I could review and simplify all code using MINGW and CYGWIN and MSC_VER. All of that code was introduced over the years. Yesterday, I stood back and asked the question "can this be simplified?". My priority at the moment is to release v0.26 and I don't want to get involved in changing anything unless it is indisputably broken.

Is there something else we must do here?

Hi Robin

So where are we now? Do you have a build issue when you build Hugin with r4764?

It build all fine on my side. Thanks again for your help.

Yesterday, I stood back and asked the question "can this be simplified?".
My priority at the moment is to release v0.26 and I don't want to get
involved in changing anything unless it is indisputably broken.

I agree. It can maybe improved. But this is no task so short before a release.

Is there something else we must do here?

From my side there is nothing open.

Good. I'm glad we're closed on this.

We're very close to release, so I don't want to change anything unless it is essential. The other concern is that it's very easy to break the build with a thing like this. We're supporting 6 different Microsoft Compilers, clang on Mac and various editions of GCC on Linux, MinGW and Cygwin. And autotools/CMake and Visual Studio solution/project files. A change can easily upset another platform. The buildserver builds and tests everything every night. However chasing down and smoothing everything is work I just don't want!

Glad we're done on this. Good Luck on Hugin. I'm sure that's another demanding effort.