You are viewing a plain text version of this content. The canonical link for it is here.
Posted to derby-dev@db.apache.org by Andrew McIntyre <mc...@gmail.com> on 2005/10/22 09:37:57 UTC
DERBY-516 patch review, pt. 1
Hi Rick,
High level comments concerning your patch for DERBY-516:
First, the requirements for the full set of compatibilty tests to run
are quite extensive. Perhaps a little more extensive than a sort-of-
casual derby user/developer might expect when trying to run a set of
tests. It is not sufficient to download the source tree and the
dependencies listed in building.txt. It requires that you have JDK
1.3, 1.4, 1.5 for your platform, and Derby jars from the 10.0 and
10.1 branches available in specific locations. I think additional
information should be added to the testing README.htm in java/testing
to include the full requirements for the compatibility test as well
as a link to the README.htm down in java/testing/.../compatibility.
At any rate, I think an effort should be made - for now - to make the
compilation and execution of these tests optional, or at the very
least, fail gracefully with informative error messages. I still
haven't been able to get just jdbcapi/CompatibilityTest to run, and
the current build/test setup doesn't give me much information to help
me figure out why.
That said:
1 - For junit.jar - the instructions in BUILDING.txt are not clear as
to what you are supposed to do with junit.jar. "Unzip it somewhere"
is not nearly specific enough. The instructions should say what file
should be put into what location. This location should be added to
extrapath.properties and that location should be referenced in any
build files that require it, as is currently the case with osgi.jar,
for example. That location should probably be tools/java since that's
where we've been putting other dependencies.
In general, compilepath.properties is reserved for JDK-related
classes, jars containing java.*, javax.*, or direct dependencies. The
new property for junit.jar should probably be called ${junit}, added
to extrapath.properties, and the build.xml in java/testing/.../
functionTests/compatibility/build.xml should include ${junit} in the
appropriate classpath entries for the applicable <javac> tasks. Same
for java/testing/.../functionTests/util/build.xml and java/
testing/.../jdbcapi/build.xml
But I really think that new build targets that are conditional on $
{junit} being <available> should be made in the above build files, in
case someone wanted to compile the non-Junit tests but didn't have
Junit - for now.
2 - empty diff in java/build/org/apache/derbyBuild/build.xml
3 - the reference to junit-3.8.1.jar in java/testing/Readme.htm
should be made more generic, and reference the convention $jardir
which is already used in that file. '/local/derby/tools/java/
junit-3.8.1.jar' not only presupposes that the user will have put
junit.jar in tools/java (which is ok, we should probably instruct the
user to put junit.jar into tools/java and reference it in
extrapath.properties from there), but into a particular path in their
filesystem -- and that particular path does not apply to all OSs.
4 - Junit noise(?) stripped in Sed.java. Is it noise? Wouldn't the
"OK..." output be useful to narrow down testcase failure?
5 - if you are going to add jdbcapi/CompatibilityTest to
derbynetmats.runall, then you should have a way for the current
harness to gracefully fail / skip the test if all the requirements of
CompatibilityTest are not met. As it is, it couldn't get jdbcapi/
CompatibilityTest to run, but it's likely that I missed something in
my setup. At any rate, I can help with sorting out any issues with
regards to getting the test to run as a part of the current test
harness.
6 - Weird diffs in build.xml (top-level) and java/client/org/apache/
derby/client/net/Reply.java
7 - The default values in testScript.xml for the locations of the
JVMs are not applicable to Mac OS X (an itchy platform for me). I'll
follow up on this later.
Since I haven't been able to get the tests to run yet, that's all I
have for now, but I'll keep at it. :-)
andrew
Re: DERBY-516 patch review, pt. 1
Posted by Andrew McIntyre <mc...@gmail.com>.
On Oct 22, 2005, at 12:37 AM, Andrew McIntyre wrote:
> 3 - the reference to junit-3.8.1.jar in java/testing/Readme.htm
> should be made more generic, and reference the convention $jardir
> which is already used in that file.
*smacks forehead* yeah, except that is the convention used in that
file for non-Derby jars. X-P
Note to self: no more email right before bed. :-)
andrew
Re: DERBY-516 patch review, pt. 1
Posted by Andrew McIntyre <mc...@gmail.com>.
On 10/25/05, David W. Van Couvering <Da...@sun.com> wrote:
> Hi, Andrew. I did create the empty file and it looks like derbyall is
> passing for me (re-running after fixing a silly misconfiguration bug).
> Are you OK with me committing and then you can work on getting it to
> work with platforms that require compile.classpath to be overriden? Or
> does this need to be fixed before checkin?
>
> Thanks,
>
> David
No, that's fine. I can check in the fix immediately after you check
the original patch in. You should send an announcement out to the list
to get everyone's attention that they'll need junit from the checkin
revision on.
andrew
Re: DERBY-516 patch review, pt. 1
Posted by "David W. Van Couvering" <Da...@Sun.COM>.
Hi, Andrew. I did create the empty file and it looks like derbyall is
passing for me (re-running after fixing a silly misconfiguration bug).
Are you OK with me committing and then you can work on getting it to
work with platforms that require compile.classpath to be overriden? Or
does this need to be fixed before checkin?
Thanks,
David
Andrew McIntyre wrote:
> On 10/24/05, Rick Hillegas <Ri...@sun.com> wrote:
>
>> I confess I don't see the value in making compilation of this test class
>>optional given that we have already voted to require JUnit as part of
>>our build machinery.
>
>
> I was quite concerned, though, when the first submission of a Junit
> test failed to compile, failed to pass in the current test harness,
> and failed (for me) to run outside of the test harness as provided.
> This certainly amplified my -0 feeling towards it - make it optional
> until we get it right. But you're absolutely right, the vote to
> require Junit passed, so, let's at least get the patch to the point
> where it doesn't break anything now and we can work on the details
> later. :-)
>
> I still currently cannot get the JDBCDriverTest class to compile. The
> problem is that compile.classpath is not guaranteed to be a certain
> level of jdk, and JDBCDriverTest needs to be compiled against JDK 1.3.
> compile.classpath needs to be overridden in certain circumstances, for
> instance when building with J2ME support, with Gump, or on Mac OS X,
> and it is not guaranteed to be the same as java13compile.classpath.
> The problem I'm currently having can be fixed by simply changing
> $[compile.classpath} in the <classpath> element of the <javac> in the
> build.xml of tests/compatibility to ${java13compile.classpath}
>
> Also, whomever is going to commit the patch needs to be sure that an
> empty master file is created in functionTests/master. My patch program
> skipped over the empty diff without creating an empty patch file.
>
>
>>>7 - The default values in testScript.xml for the locations of the
>>>JVMs are not applicable to Mac OS X (an itchy platform for me). I'll
>>>follow up on this later.
>>
>>If you can tell me what to use here, I'll make the changes.
>
>
> I'll get back to you on that. The structure of the JDK directories is
> different on Mac OS X than on Windows, Solaris or Linux, and I believe
> the same is true for some other platforms. It looks like I can work
> around it by directly setting some of the properties set at the
> beginning of the file in my ant.properties.
>
> If you can fix the first problem listed above, and resend the patch,
> then I'm ok with David or someone else committing it. Or, I can commit
> the patch as I have it modified in my local view with the one line
> difference.
>
> andrew
Re: DERBY-516 patch review, pt. 1
Posted by "David W. Van Couvering" <Da...@Sun.COM>.
Hi, Rick. Why don't we let Andrew make and test this change after I
commit, so I don't have to start all over again.
Regarding the empty master file, svn patch doesn't create empty files
for you, you have to do it manually. I have done so.
Thanks,
David
Rick Hillegas wrote:
> Hi Andrew,
>
> Thanks for being patient here. Some responses follow. Cheers-Rick
>
>
>>
>> I still currently cannot get the JDBCDriverTest class to compile. The
>> problem is that compile.classpath is not guaranteed to be a certain
>> level of jdk, and JDBCDriverTest needs to be compiled against JDK 1.3.
>> compile.classpath needs to be overridden in certain circumstances, for
>> instance when building with J2ME support, with Gump, or on Mac OS X,
>> and it is not guaranteed to be the same as java13compile.classpath.
>> The problem I'm currently having can be fixed by simply changing
>> $[compile.classpath} in the <classpath> element of the <javac> in the
>> build.xml of tests/compatibility to ${java13compile.classpath}
>>
>>
>
> I will make this change.
>
>> Also, whomever is going to commit the patch needs to be sure that an
>> empty master file is created in functionTests/master. My patch program
>> skipped over the empty diff without creating an empty patch file.
>>
>>
>
> I will have to leave this to David to verify. Unless I'm mis-reading the
> patch output, it seems that my patch does contain an empty master file.
>
>>
>>
>>>> 7 - The default values in testScript.xml for the locations of the
>>>> JVMs are not applicable to Mac OS X (an itchy platform for me). I'll
>>>> follow up on this later.
>>>>
>>>
>>> If you can tell me what to use here, I'll make the changes.
>>>
>>
>>
>> I'll get back to you on that. The structure of the JDK directories is
>> different on Mac OS X than on Windows, Solaris or Linux, and I believe
>> the same is true for some other platforms. It looks like I can work
>> around it by directly setting some of the properties set at the
>> beginning of the file in my ant.properties.
>>
>>
>
> Thanks.
>
>> If you can fix the first problem listed above, and resend the patch,
>> then I'm ok with David or someone else committing it. Or, I can commit
>> the patch as I have it modified in my local view with the one line
>> difference.
>>
>> andrew
>>
>>
>
Re: DERBY-516 patch review, pt. 1
Posted by Rick Hillegas <Ri...@Sun.COM>.
Hi Andrew,
Thanks for being patient here. Some responses follow. Cheers-Rick
>
>I still currently cannot get the JDBCDriverTest class to compile. The
>problem is that compile.classpath is not guaranteed to be a certain
>level of jdk, and JDBCDriverTest needs to be compiled against JDK 1.3.
>compile.classpath needs to be overridden in certain circumstances, for
>instance when building with J2ME support, with Gump, or on Mac OS X,
>and it is not guaranteed to be the same as java13compile.classpath.
>The problem I'm currently having can be fixed by simply changing
>$[compile.classpath} in the <classpath> element of the <javac> in the
>build.xml of tests/compatibility to ${java13compile.classpath}
>
>
I will make this change.
>Also, whomever is going to commit the patch needs to be sure that an
>empty master file is created in functionTests/master. My patch program
>skipped over the empty diff without creating an empty patch file.
>
>
I will have to leave this to David to verify. Unless I'm mis-reading the
patch output, it seems that my patch does contain an empty master file.
>
>
>>>7 - The default values in testScript.xml for the locations of the
>>>JVMs are not applicable to Mac OS X (an itchy platform for me). I'll
>>>follow up on this later.
>>>
>>>
>>If you can tell me what to use here, I'll make the changes.
>>
>>
>
>I'll get back to you on that. The structure of the JDK directories is
>different on Mac OS X than on Windows, Solaris or Linux, and I believe
>the same is true for some other platforms. It looks like I can work
>around it by directly setting some of the properties set at the
>beginning of the file in my ant.properties.
>
>
Thanks.
>If you can fix the first problem listed above, and resend the patch,
>then I'm ok with David or someone else committing it. Or, I can commit
>the patch as I have it modified in my local view with the one line
>difference.
>
>andrew
>
>
Re: DERBY-516 patch review, pt. 1
Posted by Andrew McIntyre <mc...@gmail.com>.
On 10/24/05, Rick Hillegas <Ri...@sun.com> wrote:
> I confess I don't see the value in making compilation of this test class
> optional given that we have already voted to require JUnit as part of
> our build machinery.
I was quite concerned, though, when the first submission of a Junit
test failed to compile, failed to pass in the current test harness,
and failed (for me) to run outside of the test harness as provided.
This certainly amplified my -0 feeling towards it - make it optional
until we get it right. But you're absolutely right, the vote to
require Junit passed, so, let's at least get the patch to the point
where it doesn't break anything now and we can work on the details
later. :-)
I still currently cannot get the JDBCDriverTest class to compile. The
problem is that compile.classpath is not guaranteed to be a certain
level of jdk, and JDBCDriverTest needs to be compiled against JDK 1.3.
compile.classpath needs to be overridden in certain circumstances, for
instance when building with J2ME support, with Gump, or on Mac OS X,
and it is not guaranteed to be the same as java13compile.classpath.
The problem I'm currently having can be fixed by simply changing
$[compile.classpath} in the <classpath> element of the <javac> in the
build.xml of tests/compatibility to ${java13compile.classpath}
Also, whomever is going to commit the patch needs to be sure that an
empty master file is created in functionTests/master. My patch program
skipped over the empty diff without creating an empty patch file.
> > 7 - The default values in testScript.xml for the locations of the
> > JVMs are not applicable to Mac OS X (an itchy platform for me). I'll
> > follow up on this later.
>
> If you can tell me what to use here, I'll make the changes.
I'll get back to you on that. The structure of the JDK directories is
different on Mac OS X than on Windows, Solaris or Linux, and I believe
the same is true for some other platforms. It looks like I can work
around it by directly setting some of the properties set at the
beginning of the file in my ant.properties.
If you can fix the first problem listed above, and resend the patch,
then I'm ok with David or someone else committing it. Or, I can commit
the patch as I have it modified in my local view with the one line
difference.
andrew
Re: DERBY-516 patch review, pt. 1
Posted by Rick Hillegas <Ri...@Sun.COM>.
Hi Andrew,
Thanks for reviewing this patch. Some comments follow:
Andrew McIntyre wrote:
> Hi Rick,
>
> High level comments concerning your patch for DERBY-516:
>
> First, the requirements for the full set of compatibilty tests to run
> are quite extensive. Perhaps a little more extensive than a sort-of-
> casual derby user/developer might expect when trying to run a set of
> tests. It is not sufficient to download the source tree and the
> dependencies listed in building.txt. It requires that you have JDK
> 1.3, 1.4, 1.5 for your platform, and Derby jars from the 10.0 and
> 10.1 branches available in specific locations. I think additional
> information should be added to the testing README.htm in java/testing
> to include the full requirements for the compatibility test as well
> as a link to the README.htm down in java/testing/.../compatibility.
> At any rate, I think an effort should be made - for now - to make the
> compilation and execution of these tests optional, or at the very
> least, fail gracefully with informative error messages. I still
> haven't been able to get just jdbcapi/CompatibilityTest to run, and
> the current build/test setup doesn't give me much information to help
> me figure out why.
The full compatibility test suite (all combinations) is an optional
test. Only one combination has been incorporated into derbyall. That
combination runs with the default vm against the client and server in
the trunk.
I confess I don't see the value in making compilation of this test class
optional given that we have already voted to require JUnit as part of
our build machinery. See http://wiki.apache.org/db-derby/VoteResults. I
believe you were the lone dissenter on this issue and you were kind
enough not to veto the proposal--you merely voted 0.
>
> That said:
>
> 1 - For junit.jar - the instructions in BUILDING.txt are not clear as
> to what you are supposed to do with junit.jar. "Unzip it somewhere"
> is not nearly specific enough. The instructions should say what file
> should be put into what location. This location should be added to
> extrapath.properties and that location should be referenced in any
> build files that require it, as is currently the case with osgi.jar,
> for example. That location should probably be tools/java since that's
> where we've been putting other dependencies.
I think that, according to a later posting, you found the relevant build
instructions.
>
> In general, compilepath.properties is reserved for JDK-related
> classes, jars containing java.*, javax.*, or direct dependencies. The
> new property for junit.jar should probably be called ${junit}, added
> to extrapath.properties, and the build.xml in java/testing/.../
> functionTests/compatibility/build.xml should include ${junit} in the
> appropriate classpath entries for the applicable <javac> tasks. Same
> for java/testing/.../functionTests/util/build.xml and java/
> testing/.../jdbcapi/build.xml
>
> But I really think that new build targets that are conditional on $
> {junit} being <available> should be made in the above build files, in
> case someone wanted to compile the non-Junit tests but didn't have
> Junit - for now.
I admit to being a bit muddled by this advice, given that we have voted
to make JUnit a mandatory part of our build machinery, just like ant. I
would appreciate a clarification.
>
> 2 - empty diff in java/build/org/apache/derbyBuild/build.xml
>
> 3 - the reference to junit-3.8.1.jar in java/testing/Readme.htm
> should be made more generic, and reference the convention $jardir
> which is already used in that file. '/local/derby/tools/java/
> junit-3.8.1.jar' not only presupposes that the user will have put
> junit.jar in tools/java (which is ok, we should probably instruct the
> user to put junit.jar into tools/java and reference it in
> extrapath.properties from there), but into a particular path in their
> filesystem -- and that particular path does not apply to all OSs.
Thanks, I didn't catch this one. I'll correct this reference in README.htm.
>
> 4 - Junit noise(?) stripped in Sed.java. Is it noise? Wouldn't the
> "OK..." output be useful to narrow down testcase failure?
The OK line is emitted only if the tests run cleanly. If a test fails,
it spews a diagnostic and a stack trace into the output file. This will
cause a failure-flagging diff under derbyall. All of the useful
information lives in the diagnostic and trace. I hope I'm not missing
the point of your comment.
>
> 5 - if you are going to add jdbcapi/CompatibilityTest to
> derbynetmats.runall, then you should have a way for the current
> harness to gracefully fail / skip the test if all the requirements of
> CompatibilityTest are not met. As it is, it couldn't get jdbcapi/
> CompatibilityTest to run, but it's likely that I missed something in
> my setup. At any rate, I can help with sorting out any issues with
> regards to getting the test to run as a part of the current test
> harness.
>
In a later message you found that this test requires the presence of the
db2 client on the classpath. I will fix CompatibilityTest so that it
doesn't depend on db2jcc.
> 6 - Weird diffs in build.xml (top-level) and java/client/org/apache/
> derby/client/net/Reply.java
Thanks. I don't understand these either. I'll scrub them.
>
> 7 - The default values in testScript.xml for the locations of the
> JVMs are not applicable to Mac OS X (an itchy platform for me). I'll
> follow up on this later.
>
If you can tell me what to use here, I'll make the changes.
> Since I haven't been able to get the tests to run yet, that's all I
> have for now, but I'll keep at it. :-)
>
> andrew
Re: DERBY-516 patch review, pt. 1
Posted by Andrew McIntyre <mc...@gmail.com>.
On Oct 22, 2005, at 12:37 AM, Andrew McIntyre wrote:
> 1 - For junit.jar - the instructions in BUILDING.txt are not clear
> as to what you are supposed to do with junit.jar.
> <snip>
> In general, compilepath.properties is reserved for JDK-related
> classes, jars containing java.*, javax.*, or direct dependencies.
> The new property for junit.jar should probably be called ${junit},
> added to extrapath.properties, and the build.xml in java/
> testing/.../functionTests/compatibility/build.xml should include $
> {junit} in the appropriate classpath entries for the applicable
> <javac> tasks. Same for java/testing/.../functionTests/util/
> build.xml and java/testing/.../jdbcapi/build.xml
Apparently sleepiness-induced blindness caused me to miss the (2)
part in BUILDING.txt. Sorry about that. I've added a little bit to
the description in BUILDING.txt, and fixed up the build files as
described above. I moved the section on JUnit after the JDKs, because
the default build target builds just the code for the product, and
anyone interested in building the product but not the tests could get
going with just the Ant, the JDKs, and the JDK extensions. Here's a
patch with those files, and a blank master for CompatibilityTest.out
that seemed to be missing from the original patch.