cxf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Mao <james....@iona.com>
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
Date Fri, 04 Jan 2008 06:41:22 GMT
Let's off this thread, and put the comments in the Jira CXF-1246, some 
points we made here are valuable, we should log that in the jira as well
Make sense?

James


> James Mao wrote:
>> 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.
>
> Oops, I mean *do not*
>
>
>>
>>>
>>>  
>>>> 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));
>>>>>>         
>>>
>>>
>>>
>>>   
>>
>>
>
>

Mime
View raw message