kboyd / Roc

Everything ROC and Precision-Recall curves.
BSD 2-Clause "Simplified" License
23 stars 7 forks source link

Merge makefile improvements from afb/makefile into master #21

Closed afbarnard closed 10 years ago

afbarnard commented 10 years ago

@kboyd, I fixed some issues and made some improvements to the makefile. What do you think? Ok to merge into master? (After this the plan is to merge master into dev/afbarnard and then bring all those changes into master which gives us a working main. Yay!)

kboyd commented 10 years ago

In general it looks good. I just found a few issues that we may or may not want to fix in the makefile.

  1. The following commands lead to a compilation error due to missed dependencies.
make clean
make usertests

UserScenarios.java depends on CurveTest.java (for the TOLERANCE static variable) which causes this problem. It may be easier to just give UserScenarios.java its own TOLERANCE variable (or separate out into a TestingUtils class) rather than needing to compile both UserScenarios and CurveTest, but then only run the UserScenarious class via the junit runner.

  1. Trying to use an old junit (i.e., junit4.5) can throw pretty incomprehensible errors about missing symbols that make it seem like junit isn't being found at all. This may not be worth trying to fix, but is there an easy way to check that the available junit is at least version 4.10 or such?
afbarnard commented 10 years ago
  1. If I do make clean; make usertests on 'dev/afbarnard', then CurveTest also gets compiled for some reason. (I can't figure out why.) However, if I run those make commands on 'afb/makefile', then I see the errors you mention. Perhaps it's a difference between compiling for Java 5 versus Java 7? Anyway, I will add that dependency. Thanks for catching it. I don't usually stress test the makefile. :smile:
  2. I'm not sure what to do about the JUnit version. One could probably come up with a way to check the JUnit version (e.g. by compiling and running a Java program a la autotools), but I doubt it is worth it. I think maybe it is best to document the JUnit dependency and leave it at that.
kboyd commented 10 years ago
  1. Trying to use an old junit (i.e., junit4.5) can throw pretty incomprehensible errors about missing symbols that make it seem like junit isn't being found at all. This may not be worth trying to fix, but is there an easy way to check that the available junit is at least version 4.10 or such?

Obtaining a junit version number is simple:

java -cp $(junitJars) junit.runner.Version

Parsing that to ensure it's above a particular version is going to be messy as junit is up to at least 4.12 now, so no nice string comparison. The assertArrayEquals(double[],double[],double) method seems to be primary constraint on junit version and according to http://stackoverflow.com/questions/1591415/why-does-junit4-not-have-assert-assertarrayequals-for-doubles and a quick perusal of the junit repository suggest 4.6 or above will contain the appropriate assertArrayEquals method.

afbarnard commented 10 years ago

Well, if it's that easy to get the JUnit version, then checking it is worth it. So I'll add that to this branch with documentation as well as the UserScenarios dependency.

kboyd commented 10 years ago

Perhaps it's a difference between compiling for Java 5 versus Java 7?

Very strange. I just tried a (somewhat older) Oracle Java 7 compiler (1.7.0_04) and still get the error about unable to locate CurveTest.TOLERANCE.

afbarnard commented 10 years ago

Yes, but what are your -source and -target options set to? As far as I know, that can affect how the compilation is done. (On the branch where the dependency is unnoticed, the source and target Java versions are 5.)

afbarnard commented 10 years ago

On second thought, perhaps the difference is related to whether the classes can be found in the current directory or not (i.e. when the current directory is an implied part of the classpath). Well, whatever. It's getting fixed.

kboyd commented 10 years ago

I was using javaVersion=7 and running from the afb/makefile branch at daa9fa50. In the invoked javac commands both -source and -target are set to 7.

afbarnard commented 10 years ago

I've just been experimenting, and it appears the differences are due to whether source files can be found from the current directory or not. (It appears that the current directory is effectively part of the classpath whether you specify the classpath or not.) Compare the following:

cd /path/to/Roc
cd java/src && javac -cp '' mloss/roc/Main.java  # ok
javac java/src/mloss/roc/Main.java  # fails

Part of the changes I made to the makefile include not changing directory prior to compiling, so that appears to be the reason that it works with the old makefile and not with the new makefile.

afbarnard commented 10 years ago

@kboyd, my new commits should address your concerns.

kboyd commented 10 years ago

Added a small fix so the makefile works with make setups that default to sh instead of bash.

The JUnit version checking is still not working quite right. Incorrect errors if there are multiple entries in junitJars and if the junit version is 3.?. Working on fixing them now.

kboyd commented 10 years ago

The JUnit version checking is still not working quite right. Incorrect errors if there are multiple entries in junitJars and if the junit version is 3.?.

3baa2a8 fixed these problems. The issue with the version numbering wasn't that it was 3.? but that there was a 3rd number in the version, i.e. 3.8.2. The latest commit correctly grabs the major and minor version numbers from JUnit versions of the form ?.?.? and the beta form of ?.?-beta-?.

kboyd commented 10 years ago

@afbarnard, provided my last two commits pass your inspection, I think we should merge this into master.

afbarnard commented 10 years ago

@kboyd, since you pushed commits on "my" branch, how do you want me to counter-propose changes? (I expected something more along the lines of your changes on branch kb/makefile and mine on afb/makefile and then we can exchange/merge what we decide is best.) I think we should include your fixes but I think we can do it more simply. (Particularly, I meant 'testClasspath' instead of 'junitJars', but there are a few other things I would change.)

kboyd commented 10 years ago

I'm fine with adding changes on to the end of "your" branch over whatever changes I did, but you could also make a new branch from the farthest commit that you like and cherry-pick anything else. Next time I will make my own branch.

afbarnard commented 10 years ago

I decided to keep the same pull request lineage and so just appended to the same branch. I incorporated the fixes you made in a way that was closer to my original intentions and fixes some of my original mistakes.

kboyd commented 10 years ago

Proposed a minor fix to junit version parsing in kfb/makefile. There will also be issues when/if junit moves to a different major version, but since that might also involve compatibility problems, we can deal with that when it happens.

afbarnard commented 10 years ago

Can you explain what you mean by issues when JUnit moves to a different major version?

afbarnard commented 10 years ago

I incorporated your latest fixes to JUnit version parsing. Sorry about forgetting about the dashes. (I don't have any crazy JUnit versions to test with.)

kboyd commented 10 years ago

Unless I'm misreading the bash boolean operators, a junit version of 5.1 will fail the test because 1 -ge 6 is false but junit version 5.6 would pass.

afbarnard commented 10 years ago

You are right. I'll fix right away.

kboyd commented 10 years ago

Looks good to me. I think you should merge it to master.