Skip to main content

Please review: fix for issue #126 (Exported build.xml never signs JAD files)

9 replies [Last post]
Anonymous

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
Vladimir Sizikov

Dmitri,

The changes look good. Few minor issues;

1. "Signer args may contain filenames which exists" ---> "... wich exist"

2. buildProps.store(new FileOutputStream(.....

The stream is never closed.

Thanks,
--Vladimir

On Fri, May 04, 2007 at 03:43:20PM +0400, Dmitri Trounine wrote:
> Guys,
>
> please review fix for regression #126
> (https://cqme.dev.java.net/issues/show_bug.cgi?id=126)
> More details in issue tracker.
>
> http://fisheye4.cenqua.com/changelog/cqme/branches/users/dtrounine/testE...
>
> Thanks
> Dmitri.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

oops,

On Fri, May 04, 2007 at 04:54:44AM -0700, Vladimir Sizikov wrote:
> 1. "Signer args may contain filenames which exists" ---> "... wich exist"

I made a typo myself: 'wich', shoudl be 'which exist'.

--VVS

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Dmitri Trounine

Vladimir,

Thanks for fast review!
Here is my fix:

http://fisheye4.cenqua.com/changelog/cqme/branches/users/dtrounine/testE...

Regards,
Dmitri.

Vladimir Sizikov wrote:
> oops,
>
> On Fri, May 04, 2007 at 04:54:44AM -0700, Vladimir Sizikov wrote:
>
>> 1. "Signer args may contain filenames which exists" ---> "... wich exist"
>>
>
> I made a typo myself: 'wich', shoudl be 'which exist'.
>
> --VVS
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Dmitri,

On Fri, May 04, 2007 at 04:14:43PM +0400, Dmitri Trounine wrote:
> Vladimir,
>
> Thanks for fast review!
> Here is my fix:
>
> http://fisheye4.cenqua.com/changelog/cqme/branches/users/dtrounine/testE...

You meant 836, right? :)

We're almost there. But I'd suggest to adjust the finally block in
couple of places.

First, we need to check propsOut against null, and second, catch only
IOException -- catching general Exception is not good.

Thanks,
--Vladimir

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Dmitri Trounine

Vladimir Sizikov wrote:
> You meant 836, right? :)
>
>
Right.
> We're almost there. But I'd suggest to adjust the finally block in
> couple of places.
>
> First, we need to check propsOut against null, and second, catch only
> IOException -- catching general Exception is not good.
>
>
I'm ok with changes you proposed, but just curious...

We really don't care about exceptions from closing stream, so the most
general exception can be used. Maybe even Throwable? It includes
NullPointerException and IOException.

> Thanks,
> --Vladimir
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Dmitri,

On Fri, May 04, 2007 at 04:42:32PM +0400, Dmitri Trounine wrote:
> Right.
> >We're almost there. But I'd suggest to adjust the finally block in
> >couple of places.
> >
> >First, we need to check propsOut against null, and second, catch only
> >IOException -- catching general Exception is not good.
> >
> >
> I'm ok with changes you proposed, but just curious...
>
> We really don't care about exceptions from closing stream, so the most
> general exception can be used. Maybe even Throwable? It includes
> NullPointerException and IOException.

I'm not sure that we don't care at all. Actually, if OutOfMemoryError
or AssertionError or even NPE is thrown out of close() I do care and I
would like to see at least some indication that such unexpected things
happened. This could greatly simplify debugging the problem. These
exceptions are clear indicators that something fundamental is broken,
and we'd better not hide it.

What we mostly don't care is that IOExecption from close and we can
ignore it (the exception is almost never thrown and we cannot do much
with it anyways). But to be 100% correct, I sometimes like to add something
like the following in the empty catch body:

assert false : "IOExcption from close()";

Thanks,
--Vladimir

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Dmitri Trounine

Thanks for explanation.

I've fixed finally blocks, and also added flush() just because
specification fro FileOutputStream.close doesn't say about whether
flush is called from close or note.

http://fisheye4.cenqua.com/changelog/cqme/branches/users/dtrounine/testE...

Thanks,
Dmitri.

Vladimir Sizikov wrote:
> Dmitri,
>
> On Fri, May 04, 2007 at 04:42:32PM +0400, Dmitri Trounine wrote:
>
>> Right.
>>
>>> We're almost there. But I'd suggest to adjust the finally block in
>>> couple of places.
>>>
>>> First, we need to check propsOut against null, and second, catch only
>>> IOException -- catching general Exception is not good.
>>>
>>>
>>>
>> I'm ok with changes you proposed, but just curious...
>>
>> We really don't care about exceptions from closing stream, so the most
>> general exception can be used. Maybe even Throwable? It includes
>> NullPointerException and IOException.
>>
>
> I'm not sure that we don't care at all. Actually, if OutOfMemoryError
> or AssertionError or even NPE is thrown out of close() I do care and I
> would like to see at least some indication that such unexpected things
> happened. This could greatly simplify debugging the problem. These
> exceptions are clear indicators that something fundamental is broken,
> and we'd better not hide it.
>
> What we mostly don't care is that IOExecption from close and we can
> ignore it (the exception is almost never thrown and we cannot do much
> with it anyways). But to be 100% correct, I sometimes like to add something
> like the following in the empty catch body:
>
> assert false : "IOExcption from close()";
>
> Thanks,
> --Vladimir
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
> For additional commands, e-mail: meframework-help@cqme.dev.java.net
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Vladimir Sizikov

Dmitri,

On Fri, May 04, 2007 at 05:11:35PM +0400, Dmitri Trounine wrote:
> Thanks for explanation.
>
> I've fixed finally blocks, and also added flush() just because
> specification fro FileOutputStream.close doesn't say about whether
> flush is called from close or note.
>
> http://fisheye4.cenqua.com/changelog/cqme/branches/users/dtrounine/testE...

Heheh, actually, there is no need to invoke flush() manually, it's
called from Properties.store() and even specified there:

"After the entries have been written, the output stream is flushed."

Either way is fine with me, but without extra flush() the code is more
readable. And the next time we will not be guessing why that very
strange flush() call is present... :)

Please proceed with commit, either with this code or with flush() removed.

Thanks,
--Vladimir

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net

Dmitri Trounine

Vladimir Sizikov wrote:
> Dmitri,
>
> On Fri, May 04, 2007 at 05:11:35PM +0400, Dmitri Trounine wrote:
>
>> Thanks for explanation.
>>
>> I've fixed finally blocks, and also added flush() just because
>> specification fro FileOutputStream.close doesn't say about whether
>> flush is called from close or note.
>>
>> http://fisheye4.cenqua.com/changelog/cqme/branches/users/dtrounine/testE...
>>
>
> Heheh, actually, there is no need to invoke flush() manually, it's
> called from Properties.store() and even specified there:
>
> "After the entries have been written, the output stream is flushed."
>
Oh! I should always read _all_ Javadoc specificatiions!
Thank you, Vladimir.
I'll remove flush() and then will commit.

---------------------------------------------------------------------
To unsubscribe, e-mail: meframework-unsubscribe@cqme.dev.java.net
For additional commands, e-mail: meframework-help@cqme.dev.java.net