Patch #1292

Fix targeting Windows XP (regression)

Added by Dimitri Schoolwerth 26 days ago. Updated 25 days ago.

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

100%

Category:buildEstimated time:2.00 hours
Target version:0.26

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

xp-target.patch Magnifier (1.06 KB) Dimitri Schoolwerth, 26 Apr 2017 12:56

Associated revisions

Revision 4768
Added by Robin Mills 26 days ago

#1292 Dimitri: Thank You for reporting this and providing the patch.

Revision 4769
Added by Robin Mills 26 days ago

#1292 Fix submitted.

History

#1 Updated by Robin Mills 26 days 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

#2 Updated by Robin Mills 26 days 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

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.

#3 Updated by Robin Mills 26 days 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.

#4 Updated by Robin Mills 26 days ago

  • Status changed from Assigned to Closed
  • Estimated time changed from 1.00 to 2.00

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.

#5 Updated by Dimitri Schoolwerth 25 days 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.

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.

#6 Updated by Robin Mills 25 days 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.

Also available in: Atom PDF

Redmine Appliance - Powered by TurnKey Linux