Bug #1291

MSVC: Public header pulls ins unnecessary headers and libraries

Added by T Modes 3 months ago. Updated 3 months ago.

Status:ClosedStart date:22 Apr 2017
Priority:NormalDue date:
Assignee:Robin Mills% Done:

100%

Category:buildEstimated time:5.00 hours
Target version:0.26

Description

When using libexiv2 from other program (e.g. using the shared library interface) the public header pull in now unnecessary other headers and libraries.
Unconditional the headers
#include <winsock2.h>
#include <windows.h>
#include <shlobj.h>

are pulled in from include/exiv2/config.h. But these includes are not needed in public headers! Even worst it pulls these headers also into each calling program, what is not always wanted.
The same applies that now 2 libraries are also pulled into each calling program:
#pragma comment(lib, "ws2_32.lib")
#pragma comment(lib, "wldap32.lib")

which is totally unneeded when using the shared exiv2 library.

These includes should be removed from the public headers. They should only be used in the private/internal files where needed, but not unconditional in the public headers.
Also the pragmas need to be moved into the internal source files, to correctly link the DLL.
(Maybe in static builds the pragma can be used in the public header, but only for static case. But why is then only ws2_32.lib linked this way, but not zlib.lib and xmpsdk.lib. This would be not consistent.)

incl_win.patch Magnifier - Patch (700 Bytes) T Modes, 23 Apr 2017 16:14

Associated revisions

Revision 4761
Added by Robin Mills 3 months ago

#1291 Fix submitted. Thank You to T Modes for reporting this issue.

Revision 4762
Added by Robin Mills 3 months ago

#1291 Thank you to T Modes for reporting this and providing this patch.

Revision 4763
Added by Robin Mills 3 months ago

#1291 include header simplification

Revision 4764
Added by Robin Mills 3 months ago

#1291 Reverting change in r4763 relating to windows.h as they break the Cygwin build. Retain changes in exiv2/exiv2.hpp and exv_msvc.h

History

#1 Updated by Robin Mills 3 months ago

  • Category set to build
  • Status changed from New to Assigned
  • Assignee set to Robin Mills
  • % Done changed from 0 to 10
  • Estimated time set to 2.00

T

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

To answer your question concerning the different strategy for zlib, expat and xmpsdk the honest answer is "it's been that way since the beginning of our support for MSVC". However there is another reason and that is that expat, zlib and xmpsdk are optional links. The python program configure.py can rewrite the project/solution files to omit zlib, expat and xmpsdk. Linking winsock is not optional. All exiv2 applications can perform IO with class HttpIo which requires winsock.

IO in Exiv2 is handled by the code in basicio.cpp. All versions of the library and sample applications can use the HttpIo object which requires access to WinSock and other Windows specific code. I could include those headers in BasicIo.cpp and remove them from config.h

I have a couple of questions:

1) What harm is this causing?
2) Why does this need to be changed?

Robin

#2 Updated by T Modes 3 months ago

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

You see only exiv2.dll loads WS2_32.DLL, but not exiv2.exe.

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.

#3 Updated by Robin Mills 3 months ago

  • % Done changed from 10 to 20

Thank you for a very thorough answer. I regret this has caused issues with Hugin. I will change this in the next 24 hours.

I can't even remember changing this between v0.25 and v0.26. I think I discovered a load of MSVC specific code in different files and thought "I'll consolidate this in one location and avoid duplication in various files.". However you're right, once exiv2 knows to load winsock, the calling applications do not need to link him unless they use winsock directly.

One of our commercial licensees shipped a product in March with more/or/less Exiv2 v0.26. They have a test suite of 200,000 files. I wish I'd known about this when they were doing a LOT of testing in February. None the less, your explanation is compelling and I will modify the code and run our test suite.

Thank You very much for reporting this and putting forward a strong case to support your request. It will be so!

Incidentally, I gave a 4-minute "lightning" presentation at LGM in Rio today. https://www.youtube.com/watch?v=3Fv57Lbhmqg

Robin

#4 Updated by Robin Mills 3 months ago

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

#5 Updated by T Modes 3 months ago

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)

#6 Updated by Robin Mills 3 months ago

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

#7 Updated by T Modes 3 months ago

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.

#8 Updated by Robin Mills 3 months ago

Ah. My recommended way to use exiv2 is:

#include <exiv2/exiv2.hpp>
That's what all our sample code does.

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.

#9 Updated by Robin Mills 3 months ago

  • % Done changed from 20 to 80
  • Estimated time changed from 2.00 to 4.00

#10 Updated by T Modes 3 months ago

I have attached the patch, that works for me. (It's mainly in the form you have already written.)

Ah. My recommended way to use exiv2 is:

#include <exiv2/exiv2.hpp>

That's what all our sample code does.

I inherited the code. It is long time in this form. And there are many other libraries used by Hugin, so often there are conflicts/interferences between them (often only a small change can make a difference).

#11 Updated by Robin Mills 3 months ago

  • Status changed from Assigned to Closed
  • % Done changed from 80 to 100

Fix submitted r4762. I reverted my local changes and submitted your patch. Your changes and mine seem to be identical.

Thank You for working with me on this. In software: 1+1=3.

The include <foo> is a preprocessor monstrosity. Tiny changes in order of includes can be devastating and very difficult to debug. That's why I modified all our samples to use include <exiv2/exiv2.hpp>. If you only need to include one thing, what can go wrong? (answer: many many things can go wrong).

<windows.h> is now only included in one library header:

517 rmills@rmillsmbp:~/gnu/exiv2/trunk $ for d in src samples include ; do find $d -name "*hpp" -exec grep -H windows\.h {} \; ; done
src/exiv2app.hpp:#include <windows.h>
include/exiv2/rwlock.hpp:#include <windows.h>
518 rmills@rmillsmbp:~/gnu/exiv2/trunk $ 

Incidentally, I see windows.h is included in loads of places. I've been working on this code for 9 years and I learn new things about it every day.

515 rmills@rmillsmbp:~/gnu/exiv2/trunk $ for d in src samples include ; do find $d -name "*pp" -exec grep -H windows\.h {} \; ; done
src/basicio.cpp:# include <windows.h>
src/convert.cpp:# include <windows.h>
src/exiv2app.hpp:#include <windows.h>
src/http.cpp:#include <windows.h>
src/jpgimage.cpp:#include <windows.h>
src/makernote.cpp:#include <windows.h>
src/types.cpp:# include <windows.h> // for MultiByteToWideChar etc
src/version.cpp:#include <windows.h>
src/version.cpp:# include <windows.h>
samples/exiv2json.cpp:#include <windows.h>
samples/geotag.cpp:#include <windows.h>
include/exiv2/rwlock.hpp:#include <windows.h>
516 rmills@rmillsmbp:~/gnu/exiv2/trunk $ 
I think I'll remove all those pointless references in the .cpp files.

#12 Updated by Robin Mills 3 months ago

  • Estimated time changed from 4.00 to 5.00

r4763

1) Reviewed and simplified references to windows.h.
2) modified exiv2/exiv2.hpp to reference files such as exiv2/types.hpp instead of types.hpp. This will eliminate the possibility that types.hpp is read from a totally different library.
3) Bumped the version to 0.26 in exv_msvc.h and exv_msvc-webready

T:
Can you build Hugin with this set of headers, please?
If you're good, I won't touch this stuff any more (unless there's a build issue on another platform such as CYGWIN or MINGW).

#13 Updated by Robin Mills 3 months ago

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.

#14 Updated by T Modes 3 months ago

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

#15 Updated by Robin Mills 3 months ago

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 believe the code you mention (involving MINGW and CYGWIN) resides in basicio.cpp (and something similar in actions.cpp, exiv2.cpp, http.cpp, makernote.cpp, utils.cpp, minoltamn.cpp and version.cpp) In r4763, I attempted to simplify and broke the build. So in r4764, I reverted the simplification.

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?

#16 Updated by T Modes 3 months ago

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.

#17 Updated by Robin Mills 3 months ago

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.

Also available in: Atom PDF

Redmine Appliance - Powered by TurnKey Linux