Patch #1292
Fix targeting Windows XP (regression)
100%
Description
SRW lock functions are used since r4308, these are only available since Windows Vista and some since Windows 7. The attached patch fixes XP support when using VS2013+, yet still targeting XP.
Note that this doesn't solve compatibility with Vista when XP is not targeted (which is the default). Ideally availability of the SRW lock functions would be checked at run-time but given the desired performance nature of this piece of code that may not be wanted (I didn't profile).
Quite frankly I would probably just always use the critical section part of the code, unless it's been shown that using those functions are reason for real world performance concerns.
Random commit message suggestion (no need for possible thanks (only), I would rather just have more sensible commit messages if that's OK) :
"
Fix targeting XP systems
SRW lock functions are being used which are only available since Windows 7. Fix by not making use of them when targeting XP.
Regression since r4308.
"
Files
History
Updated by Robin Mills over 4 years ago
Dimitri
I thought I had to do something like this to build with Visual Studio 2003 (which will be deprecated after v0.26 ships). Do you think there is a risk to the build here? Perhaps it's better to leave this as a patch for the occasional user of XP. What do you think?
Robin
Updated by Robin Mills over 4 years ago
- Status changed from New to Assigned
- Target version set to 0.26
- % Done changed from 0 to 100
- Estimated time set to 1.00 h
Thank You, Dimitri for reporting this and providing the patch. I've reviewed this and submitted this: r4768
I'm going to accept it and do a full build (2005/8/10/12/13/15). If anything breaks, I will revert the patch.
Updated by Robin Mills over 4 years ago
By the way, the reason the code uses SRWLOCK is because it was a contribution from a user. Then I discover it doesn't work on older version of Windows - and other consequences. I have a strong dislike of locks. Multi-threading is a very tricky beast and very difficult to test. And of course a host application could deadlock. Anyway, it's in the code for v0.26.
Updated by Robin Mills over 4 years ago
- Status changed from Assigned to Closed
- Estimated time changed from 1.00 h to 2.00 h
This patch has successfully built and passed the test suite on all 17 Supported Platforms which are:
12 builds: MSVC {2005/8/10/12/13/15}x{ 32/64 } dll
1 build: MSVC2003/32 static
1 build: Linux/64/GCC5.2 shared
1 build: Linux/64/GCC4.9 shared
1 build: MacOSX/64/clang dylib
1 build: MinGW/32/GCC4.9 static
Congrats. I hope this is the final patch for v0.26. Hoping to ship v0.26 tonight.
Updated by Dimitri Schoolwerth over 4 years ago
Thank you Robin (and for all of your work!).
Robin Mills wrote:
I thought I had to do something like this to build with Visual Studio 2003 (which will be deprecated after v0.26 ships). Do you think there is a risk to the build here?
I don't think there's a risk. Even if the 'wrong' branch would be picked it still works: The #else branch (with critical section code) is sure to work since at least Windows 95 (which is not supported, but just for reference) and with very old SDKs too. The only reason the SRW lock branch is there is for (profiled?) performance reasons, according to note #1187-29.
Perhaps it's better to leave this as a patch for the occasional user of XP. What do you think?
That's already what it is supposed to do: Only if someone using a newer VS explicitly targets XP then the critical section code is used which is compatible with releases prior to Windows 7.
Which VS version is used to create the downloadable Windows binary by the way, VS2003? If it's VS2012 (actually it depends on SDK version, not so much VS version) or earlier than the code will work with XP and Vista. In other cases Exiv2 will require at least Windows 7 by default, unlike 0.25.
Updated by Robin Mills over 4 years ago
Thanks, Dimitri. I asked for your "risk assessment" before I reviewed your patch. We agree that your patch is good.
Exiv2 v0.26 will publish builds from the buildserver for 16 platforms. I believe the VS2005/32 dll build will work on XP.
12 builds: MSVC {2005/8/10/12/13/15}x{ 32/64 } dll
1 build: Linux/64/GCC5.2 shared
1 build: Cygwin/64/GCC4.9 shared
1 build: MacOSX/64/clang dylib
1 build: MinGW/32/GCC4.9 static
As you know, Exiv2 is open-source. We publish source code. Binary builds in the release are provided for user convenience. "VS 2003 .Net is deprecated", means that I do not actively build and test that platform. However the source code probably works. After v0.26, I intend to remove the msvc2003 directory and move forward with only the msvc directory which builds successfully with VS: {2005|8|10|12|13|15|17} x {dll|static} x {32|64} x {debug|release} (56 configurations).
I will continue to support Linux, Cygwin and MacOS-X with numerous versions of GCC and clang.
The MinGW msys/1.0 platform seems in poor shape and will be deprecated after Exiv2 v0.26 ships. Please remember "deprecated" = I don't build and test this platform. However it probably works.
Updated by Robin Mills almost 4 years ago
Dimitri
Last year we discussed the project to "fix 64 bit I/O in Exiv2" and I believe you said "Maybe in 2018". So we're now is 2018 and probably even busier than 2017. Are you still interested?
I'm glad to say that I now have a little team of 4 helping me with Exiv2. I've proposed to hold a developer's meeting in May: https://github.com/Exiv2/exiv2/issues/225
There are many interesting projects which could be usefully added to Exiv2. The puzzle for the future is to find people willing to commit the time to develop and support the library. I am 67 years old and announced my retirement last year. I'm still contributing and very pleased that, after years on my own, others are actively contributing to Exiv2. http://dev.exiv2.org/boards/3/topics/2992
#1292 Dimitri: Thank You for reporting this and providing the patch.