You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Conor MacNeill <co...@cenqua.com> on 2007/05/29 07:38:04 UTC
[PATCH] Better reporting of JavaHL loading exceptions
Hi,
This change retains the first thrown exception when attempting to load
the svnjavahl-1 native library rather than throwing the exception from
the last attempt. This is important when the user specifies the library
to load and it cannot be loaded. At present the error that finally gets
returned to the user is not the one related to the attempt to load their
library.
On my system for example, the change lets me see this error:
Could not load svn-javahl:
java.lang.UnsatisfiedLinkError-C:\Software\svn-dev\javahl\libsvnjavahl-1.dll:
Can't load IA 32-bit .dll on a AMD 64-bit platform
which before I could not. Here is my suggested log message:
[[[
Better error reporting when unable to load the Native JavaHL library
*
subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeResources.java
(loadNativeLibrary): Use the earliest exception to report to the user
rather than the last
]]]
Let me know if I goofed.
Conor
Re: [PATCH] Better reporting of JavaHL loading exceptions
Posted by Blair Zajac <bl...@orcaware.com>.
Daniel Rall wrote:
> On Tue, 29 May 2007, Conor MacNeill wrote:
>
>> This change retains the first thrown exception when attempting to load
>> the svnjavahl-1 native library rather than throwing the exception from
>> the last attempt. This is important when the user specifies the library
>> to load and it cannot be loaded. At present the error that finally gets
>> returned to the user is not the one related to the attempt to load their
>> library.
>>
>> On my system for example, the change lets me see this error:
>>
>> Could not load svn-javahl:
>> java.lang.UnsatisfiedLinkError-C:\Software\svn-dev\javahl\libsvnjavahl-1.dll:
>> Can't load IA 32-bit .dll on a AMD 64-bit platform
>
> Sounds like a good idea, Connor.
>
>> which before I could not. Here is my suggested log message:
>>
>> [[[
>> Better error reporting when unable to load the Native JavaHL library
>>
>> *
>> subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeResources.java
>> (loadNativeLibrary): Use the earliest exception to report to the user
>> rather than the last
>> ]]]
>>
>> Let me know if I goofed.
>
> A few issues:
>
> - You continue to use the "if (loadException == null)" check after the
> first two attempts to load the library. These will never actually
> succeed in setting "loadException" though, will they? (Just as the
> second won't succeed if the "subversion.native.library" system
> property is set.)
>
> - You unrolled the exception handling from the nested try/catch blocks
> which were previously used. While not necessarily a bad change, this
> formatting change makes the diff harder to read and review.
>
> - You used tabs instead of spaces, which makes the diff more difficult
> to review, and don't conform to the Subversion project's coding
> conventions.
>
> I'm attaching a revised patch which attempts to address the second two
> items. I'm not sure the first is worth addressing. Thoughts?
>
>
> ------------------------------------------------------------------------
>
> Index: subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeResources.java
> ===================================================================
> --- subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeResources.java (revision 25190)
> +++ subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeResources.java (working copy)
> @@ -47,6 +47,8 @@
> */
> public static synchronized void loadNativeLibrary()
> {
> + UnsatisfiedLinkError loadException = null;
> +
> // If the user specified the fully qualified path to the
> // native library, try loading that first.
> try
> @@ -62,32 +64,39 @@
> }
> catch (UnsatisfiedLinkError ex)
> {
> - // ignore that error to try again
> + // Since the user explicitly specified this path, this is
> + // the best error to return if no other method succeeds.
> + loadException = ex;
> }
>
> // Try to load the library by its name. Failing that, try to
> // load it by its old name.
> - try
> + String[] libraryNames = {"svnjavahl-1", "libsvnjavahl-1", "svnjavahl"};
> + for (int i = 0; i < libraryNames.length; i++)
> {
> - System.loadLibrary("svnjavahl-1");
> - init();
> - return;
> - }
> - catch (UnsatisfiedLinkError ex)
> - {
> try
> {
> - System.loadLibrary("libsvnjavahl-1");
> + System.loadLibrary(libraryNames[i]);
> init();
> return;
> }
> - catch (UnsatisfiedLinkError e)
> + catch (UnsatisfiedLinkError ex)
> {
> - System.loadLibrary("svnjavahl");
> - init();
> - return;
> + if (loadException == null)
> + {
> + loadException = ex;
> + }
> }
> }
> +
> + // Re-throw the most relevant exception.
> + if (loadException == null)
> + {
> + // This could only happen as the result of a programming error.
> + loadException = new UnsatisfiedLinkError("Unable to load JavaHL " +
> + "native library");
> + }
> + throw loadException;
> }
>
> /**
I like the idea of the patch.
How about using
if (null == loadException)
instead of
if (loadException == null)
This style of coding prevents the accidental assignment of null to
loadException.
Regards,
Blair
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Better reporting of JavaHL loading exceptions
Posted by Daniel Rall <dl...@collab.net>.
On Wed, 30 May 2007, Conor MacNeill wrote:
> Daniel,
>
> On 30/05/07, Daniel Rall <dl...@collab.net> wrote:
> >
> >
> >A few issues:
> >
> >- You continue to use the "if (loadException == null)" check after the
> >first two attempts to load the library. These will never actually
> >succeed in setting "loadException" though, will they? (Just as the
> >second won't succeed if the "subversion.native.library" system
> >property is set.)
>
>
> true
>
>
> - You unrolled the exception handling from the nested try/catch blocks
> >which were previously used. While not necessarily a bad change, this
> >formatting change makes the diff harder to read and review.
>
>
> I thought it was clearer not to keep going down nested try/catch especially
> as the throw out of the most nested catch was not really obvious. I think
> the for loop approach in your version is better anyway.
>
>
> - You used tabs instead of spaces, which makes the diff more difficult
> >to review, and don't conform to the Subversion project's coding
> >conventions.
>
>
>
> Sorry about that - don't normally use Visual Studio for Java editing and
> only set it to use spaces half way through making the change. Will be more
> careful in future.
>
>
> I'm attaching a revised patch which attempts to address the second two
> >items. I'm not sure the first is worth addressing. Thoughts?
> >
> >
> +1 from me
Okay, committed to trunk in r25197.
Re: [PATCH] Better reporting of JavaHL loading exceptions
Posted by Conor MacNeill <co...@cenqua.com>.
Daniel,
On 30/05/07, Daniel Rall <dl...@collab.net> wrote:
>
>
> A few issues:
>
> - You continue to use the "if (loadException == null)" check after the
> first two attempts to load the library. These will never actually
> succeed in setting "loadException" though, will they? (Just as the
> second won't succeed if the "subversion.native.library" system
> property is set.)
true
- You unrolled the exception handling from the nested try/catch blocks
> which were previously used. While not necessarily a bad change, this
> formatting change makes the diff harder to read and review.
I thought it was clearer not to keep going down nested try/catch especially
as the throw out of the most nested catch was not really obvious. I think
the for loop approach in your version is better anyway.
- You used tabs instead of spaces, which makes the diff more difficult
> to review, and don't conform to the Subversion project's coding
> conventions.
Sorry about that - don't normally use Visual Studio for Java editing and
only set it to use spaces half way through making the change. Will be more
careful in future.
I'm attaching a revised patch which attempts to address the second two
> items. I'm not sure the first is worth addressing. Thoughts?
>
>
+1 from me
Conor
Re: [PATCH] Better reporting of JavaHL loading exceptions
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 29 May 2007, Conor MacNeill wrote:
> This change retains the first thrown exception when attempting to load
> the svnjavahl-1 native library rather than throwing the exception from
> the last attempt. This is important when the user specifies the library
> to load and it cannot be loaded. At present the error that finally gets
> returned to the user is not the one related to the attempt to load their
> library.
>
> On my system for example, the change lets me see this error:
>
> Could not load svn-javahl:
> java.lang.UnsatisfiedLinkError-C:\Software\svn-dev\javahl\libsvnjavahl-1.dll:
> Can't load IA 32-bit .dll on a AMD 64-bit platform
Sounds like a good idea, Connor.
> which before I could not. Here is my suggested log message:
>
> [[[
> Better error reporting when unable to load the Native JavaHL library
>
> *
> subversion/bindings/javahl/src/org/tigris/subversion/javahl/NativeResources.java
> (loadNativeLibrary): Use the earliest exception to report to the user
> rather than the last
> ]]]
>
> Let me know if I goofed.
A few issues:
- You continue to use the "if (loadException == null)" check after the
first two attempts to load the library. These will never actually
succeed in setting "loadException" though, will they? (Just as the
second won't succeed if the "subversion.native.library" system
property is set.)
- You unrolled the exception handling from the nested try/catch blocks
which were previously used. While not necessarily a bad change, this
formatting change makes the diff harder to read and review.
- You used tabs instead of spaces, which makes the diff more difficult
to review, and don't conform to the Subversion project's coding
conventions.
I'm attaching a revised patch which attempts to address the second two
items. I'm not sure the first is worth addressing. Thoughts?