Mailing-List: contact cxf-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: cxf-dev@incubator.apache.org
Received-SPF: pass (nike.apache.org: domain of james.mao@iona.com designates
 12.170.54.180 as permitted sender)
Message-ID: <477DADDC.4040704@iona.com>
Date: Fri, 04 Jan 2008 11:54:04 +0800
From: James Mao <james.mao@iona.com>
User-Agent: Thunderbird 2.0.0.9 (Windows/20071031)
MIME-Version: 1.0
To: Daniel Kulp <dkulp@apache.org>
CC: cxf-dev@incubator.apache.org
Subject: Re: svn commit: r607390 - in
 /incubator/cxf/trunk/maven-plugins/codegen-plugin/src/main/java/org/apache/cxf/maven_plugin:
 WSDL2JavaMojo.java WsdlOptionLoader.java
References: <200801031101.47968.dkulp@apache.org> <477DA0CF.5070201@iona.com>
 <200801032231.22028.dkulp@apache.org>
In-Reply-To: <200801032231.22028.dkulp@apache.org>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Dan,

> On Thursday 03 January 2008, James Mao wrote:
>   
>>> This commit is breaks all kinds of things and I think needs to be
>>> rolled back.   I certainly need to roll it back off the 2.0.4
>>> branch.
>>>
>>> Problems I see:
>>> 1) By default, it grabs *.wsdl from src/main/resources/wsdl to
>>> process. This isn't backwords compatible at all so I need to roll
>>> that off the 2.0.4 branch.  For example, the 2.0.4 testutils fails
>>> to build with it as there are wsdls in there (the catalog stuff)
>>> that cannot/shouldnot be processed.
>>>       
>> By default, it grabs *.wsdl from the $wsdlRoot, unless it's not set
>>     
>
> No, it grabs it from src/main/resources/wsdl if it's not set.   That's 
> the problem.   If it didn't do anything if wsdlRoot wasn't set, I'd be 
> OK with it.
>   

Previously, yes, it won't do anything if wsdlRoot is not set.
but in this case if you do mvn wsdl2java:wsdl2java, nothing will happen.
and this is what I need to fix for the CXF-1246

>   
>> I have moved those shouldnot/cannot be processed wsdls out of
>> testutils to the places that they should belong,
>> To put all of the wsdls in testutils does not make any sense to me.
>>     
>
> Which is fine for 2.1, but not for 2.0.4.   If the change causes our own 
> build to break, it's not a backword compatible change and not something 
> we should put on the 2.0.x patches branch.    Our users could have 
> similar setups and updating to 2.0.4 should cause strange breakages.
>   

Sorry, I probably will not care too much about the 2.0.4, as I commented 
in the CXF-1246, it is for the CXF 2.1 release.
So, please do merge anything to 2.0.4 branch, that definitely will have 
a lot of work if we do so.

>
>   
>> I guess 2.0.4 could do the same thing? I can merge the commits to
>> 2.0.4 if we would like to do that.
>>
>> If not, just use the <wsdlOption/>s to specify the wsdls you want to
>> build, and set the wsdlRoot to '/dev/null' to turn off the auto
>> loading.
>>     
>
> Again, that's not a backword compatible thing.  
>
> I think what we need to do for 2.0.4 is make it NOT scan anything if 
> wsdlRoot is not set.  Provide an expression to set it when calling from 
> the command line.
>
> Basically just:
>    /**
>      * @parameter expression="${wsdl2java.wsdlRoot}"
>      */
>     File wsdlRoot;
>
>
>   
>>> 3) defaults should be set with default-value tags on the @parameter
>>> so they are documented.   They shouldnt be in the code.   Probably
>>> should have a separate var for testWsdlRoot.
>>>       
>> I will think about it, but since we are using maven, and so I prefer
>> the so-called CoC principle,
>> Hard code is not always a bad thing. I personally prefer less options
>>     
>
> Umm....  we are using maven and thus we should be following the same 
> practices that maven uses, which is to use the flags on the @parameter 
> stuff and let plexus inject it.  
>   

But no default value should be set on the @parameter, we should hard 
code it if it's null, right?

James

> Dan
>
>   
>>> 4) They probably should be a File object, not a String.
>>>
>>>
>>> For example:
>>>     /**
>>>      * @parameter expression="${wsdl2java.wsdlRoot}"
>>> default-value="${basedir}/src/main/resources/wsdl"
>>>      */
>>>     File wsdlRoot;
>>>
>>> That would provide a good default, but also also setting it via a
>>> command line:
>>>
>>> mvn codegen-plugin:wsdl2java -Dwsdl2java.wsdlRoot=target/foo
>>>
>>> Actually, sourceRoot and testSourceRoot should probably have
>>> expressions to make them settable on the command line as well.
>>>
>>>
>>> Looking at it, I'm wondering if we should make the mojo abstract and
>>> make two subclasses: one for source and one for tests.
>>> wsdl2java
>>> wsdl2javatests
>>> or similar.   It would definitely simplify it quite a bit.
>>>
>>> Dan
>>>
>>> mmao wrote:
>>>       
>>>> Author: mmao
>>>> Date: Fri Dec 28 23:18:46 2007
>>>> New Revision: 607390
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=607390&view=rev
>>>> Log:
>>>> CXF-1246
>>>>   Support run wsdl2java in the style of
>>>>      mvn
>>>> org.apache.cxf:cxf-codegen-plugin:2.1-incubator-SNAPSHOT:wsdl2java
>>>>
>>>>
>>>> Modified:
>>>>         
>>> incubator/cxf/trunk/maven-plugins/codegen-plugin/src/main/java/org/a
>>> pache/cxf/maven_plugin/WSDL2JavaMojo.java
>>>
>>>
>>>
>>>
>>> incubator/cxf/trunk/maven-plugins/codegen-plugin/src/main/java/org/a
>>> pache/cxf/maven_plugin/WsdlOptionLoader.java
>>>
>>>       
>>>> Modified:
>>>>         
>>> incubator/cxf/trunk/maven-plugins/codegen-plugin/src/main/java/org/a
>>> pache/cxf/maven_plugin/WSDL2JavaMojo.java
>>>
>>>       
>>>> URL:
>>>>         
>>> http://svn.apache.org/viewvc/incubator/cxf/trunk/maven-plugins/codeg
>>> en-plugin/src/main/java/org/apache/cxf/maven_plugin/WSDL2JavaMojo.jav
>>> a?rev=607390&r1=607389&r2=607390&view=diff
>>>
>>> ====================================================================
>>> ==========
>>>
>>>       
>>>> ---
>>>>         
>>> incubator/cxf/trunk/maven-plugins/codegen-plugin/src/main/java/org/a
>>> pache/cxf/maven_plugin/WSDL2JavaMojo.java
>>>
>>>       
>>>> (original)
>>>> +++
>>>>         
>>> incubator/cxf/trunk/maven-plugins/codegen-plugin/src/main/java/org/a
>>> pache/cxf/maven_plugin/WSDL2JavaMojo.java
>>>
>>>       
>>>> Fri Dec 28 23:18:46 2007
>>>> @@ -87,6 +87,16 @@
>>>>       */
>>>>      boolean useCompileClasspath;
>>>>
>>>> +    private List<WsdlOption> getWsdlOptionsFromDir(final File
>>>> root) throws MojoExecutionException {
>>>> +        List<WsdlOption> options = new ArrayList<WsdlOption>();
>>>> +        for (WsdlOption o : new WsdlOptionLoader().load(root)) {
>>>> +            if (!options.contains(o)) {
>>>> +                options.add(o);
>>>> +            }
>>>> +        }
>>>> +        return options;
>>>> +    }
>>>> +
>>>>      public void execute() throws MojoExecutionException {
>>>>          String outputDir = testSourceRoot == null ? sourceRoot :
>>>> testSourceRoot;
>>>>          File outputDirFile = new File(outputDir);
>>>> @@ -95,22 +105,25 @@
>>>>          File classesDir = new File(classesDirectory);
>>>>          classesDir.mkdirs();
>>>>
>>>> -
>>>> -        if (wsdlRoot != null) {
>>>> -            List<WsdlOption> options = new
>>>> ArrayList<WsdlOption>(); -            if (wsdlOptions != null) {
>>>> -                options.addAll(Arrays.asList(wsdlOptions));
>>>> +        List<WsdlOption> options = new ArrayList<WsdlOption>();
>>>> +        if (wsdlRoot == null) {
>>>> +            File sourceWsdlRoot = new File(project.getBasedir(),
>>>> "/src/main/resources/wsdl");
>>>> +            if (sourceWsdlRoot.exists()) {
>>>> +               
>>>> options.addAll(getWsdlOptionsFromDir(sourceWsdlRoot)); }
>>>> -
>>>> -            for (WsdlOption o : new
>>>> WsdlOptionLoader().load(wsdlRoot))
>>>>         
>>> {
>>>
>>>       
>>>> -                if (!options.contains(o)) {
>>>> -                    options.add(o);
>>>> -                }
>>>> +
>>>> +            File testWsdlRoot = new File(project.getBasedir(),
>>>> "/src/test/resources/wsdl");
>>>> +            if (testWsdlRoot.exists()) {
>>>> +               
>>>> options.addAll(getWsdlOptionsFromDir(testWsdlRoot)); }
>>>> -            wsdlOptions = options.toArray(new
>>>> WsdlOption[options.size()]);
>>>>          }
>>>>
>>>> -        if (wsdlOptions == null) {
>>>> +        if (wsdlOptions != null) {
>>>> +            options.addAll(Arrays.asList(wsdlOptions));
>>>> +        }
>>>> +        wsdlOptions = options.toArray(new
>>>> WsdlOption[options.size()]); +
>>>> +        if (wsdlOptions.length == 0) {
>>>>              getLog().info("Nothing to generate");
>>>>              return;
>>>>          }
>>>>
>>>> Modified:
>>>>         
>>> incubator/cxf/trunk/maven-plugins/codegen-plugin/src/main/java/org/a
>>> pache/cxf/maven_plugin/WsdlOptionLoader.java
>>>
>>>       
>>>> URL:
>>>>         
>>> http://svn.apache.org/viewvc/incubator/cxf/trunk/maven-plugins/codeg
>>> en-plugin/src/main/java/org/apache/cxf/maven_plugin/WsdlOptionLoader.
>>> java?rev=607390&r1=607389&r2=607390&view=diff
>>>
>>> ====================================================================
>>> ==========
>>>
>>>       
>>>> ---
>>>>         
>>> incubator/cxf/trunk/maven-plugins/codegen-plugin/src/main/java/org/a
>>> pache/cxf/maven_plugin/WsdlOptionLoader.java
>>>
>>>       
>>>> (original)
>>>> +++
>>>>         
>>> incubator/cxf/trunk/maven-plugins/codegen-plugin/src/main/java/org/a
>>> pache/cxf/maven_plugin/WsdlOptionLoader.java
>>>
>>>       
>>>> Fri Dec 28 23:18:46 2007
>>>> @@ -38,14 +38,16 @@
>>>>      private static final String WSDL_BINDINGS =
>>>> "-binding-?\\d*.xml$";
>>>>
>>>>      public List<WsdlOption> load(String wsdlRoot) throws
>>>> MojoExecutionException {
>>>> -        File wsdlBasedir;
>>>> -        if (wsdlRoot == null) {
>>>> +        return load(new File(wsdlRoot));
>>>> +    }
>>>> +
>>>> +    public List<WsdlOption> load(File wsdlBasedir) throws
>>>> MojoExecutionException {
>>>> +        if (wsdlBasedir == null) {
>>>>              return new ArrayList<WsdlOption>();
>>>>          }
>>>> -        wsdlBasedir = new File(wsdlRoot);
>>>>
>>>>          if (!wsdlBasedir.exists()) {
>>>> -            throw new MojoExecutionException(wsdlRoot + " not
>>>>         
>>> exists");
>>>
>>>       
>>>> +            throw new MojoExecutionException(wsdlBasedir + " not
>>>> exists");
>>>>          }
>>>>
>>>>          return findJobs(wsdlBasedir, getWsdlFiles(wsdlBasedir));
>>>>         
>
>
>
>