[7u4] Review request for CR 7134730

[Greg Brown]

> 1. General comments the copyright is not formatted right, it is now:
> 2 * Copyright 2012 Oracle and/or its affiliates. All rights reserved.

Thanks - I'll update all of the headers.

> 2.  main.m // disclaimer I don't fully understand this language (Objective C ?)
>     FULL_VERSION and DOT_VERSION is always  is hard-coded to
>     1.7.0 what happens when we go to 1.8.0 ?  Can we not set this
>     in some automated way.

This is a temporary workaround - see the TODO in main.m re: using a makefile.

> 3. It looks the cp_wildcard expansion is set to TRUE in JLI_LaunchfFxnPtr, do
>   we really need this ? the classpath is going to be hard-coded right ? IMO
>   we don't require cp wildcarding, we can set that to FALSE.

Thanks for catching that - I had originally planned to use wildcard expansion, but the current implementation does not do that. I will set it to FALSE.

> 4. I also find the version string hard-coded in build.xml, we need to have a mechanism
>   such that version information is derived from the make/build logic.

Do you mean the version that we get from build.properties?

> 5. The test  is great, but how will the test be executed ? typically
>   in the jdk, the tests are located under jdk/test/......, and these are all run by
>   jtreg automatically on a periodic basis (nightly, weekly, promotion etc), we
>  need this hooked up into that framework.

You are correct - enabling automated unit tests for this code is still a TODO.

> 6. AppBundlerTask.java.html
> 
> Does the ant code get built and run with jdk6 ?

No, Java 7 is required.

> it that is the case then the following
> may be moot, but of course you can use for-each and simplify some code below.
> 
>  71     private ArrayList<File>  classPath = new ArrayList<File>();
>  72     private ArrayList<File>  nativeLibraries = new ArrayList<File>();
>  73     private ArrayList<String>  options = new ArrayList<String>();
>  74     private ArrayList<String>  arguments = new ArrayList<String>();
> 
> you can set the assiged type to List.
> private List<File> classPath = new ArrayList<>();

That's true. I haven't fully acclimated myself to Java 7 syntax yet.  :-)

> lines 135....
> 
> 135     public void addConfiguredClassPath(FileSet classPath) {
> 136         File parent = classPath.getDir();
> 137
> 138         DirectoryScanner directoryScanner = classPath.getDirectoryScanner(getProject());
> -139         String[] includedFiles = directoryScanner.getIncludedFiles();
> 140
> -141         for (int i = 0; i<  includedFiles.length; i++) {
> -142             this.classPath.add(new File(parent, includedFiles[i]));
> -143         }
> 	      for (String name : directoryScanner.getIncludedFiles() {
> 		  this.classPath.add(new File(parent, name);
>              }
>  144     }

Thanks for this suggestion. Apparently I'm not yet fully acclimated to Java 7 APIs, either.  ;-)

> similarly lines 453 and 477.
> 
> Test.java, I would make the test program dump out the args as well,
> and pass some dummy args to ensure that args are passed correctly
> to the app's main.

Good idea.

> I noticed that you set the starting heapsize and maximum heap size ?

That's just to test setting VM options using the launcher. I can change them to something else if there are better test options to use.

G