2 maven-exec-plugin patches

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

2 maven-exec-plugin patches

Trask Stalnaker
Hi,

I have submitted 2 patches to the maven-exec-plugin project.  What's the best way to help move these forward?



Thanks,
Trask

Reply | Threaded
Open this post in threaded view
|

Re: 2 maven-exec-plugin patches

Robert Scholte-3
Hi,

thanks for the patches.
A few remarks: Even though JDK6 has reached EOL, Mavens requirement is  
still JDK5.
So for MEXEC-121 you need to check if java.io.Console is available, since  
it was introduced with Java6

In both cases it would be nice to add an integration test, based on the  
maven-invoker-plugin[1], to confirm your solution.
There are already several tests under src/it
Run 'mvn verify' to confirm that everything still works.

thanks,
Robert

[1] http://maven.apache.org/plugins/maven-invoker-plugin/


Op Sat, 04 Jan 2014 23:54:59 +0100 schreef Trask Stalnaker  
<[hidden email]>:

> Hi,
>
> I have submitted 2 patches to the maven-exec-plugin project.  What's the
> best way to help move these forward?
>
> http://jira.codehaus.org/browse/MEXEC-118
>
> http://jira.codehaus.org/browse/MEXEC-121
>
> Thanks,
> Trask

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


Reply | Threaded
Open this post in threaded view
|

Re: 2 maven-exec-plugin patches

Trask Stalnaker
Hi Robert,

Thanks for the response.

The patch for MEXEC-121 already checks whether System.console() exists and falls back to System.out if it does not (i.e. on JDK5).

I looked at the test harness (invoker).  I'm not sure I can use it to test MEXEC-121, since the test harness captures the output, and so System.console() will return null and the patch will fall back to using System.out in this case.  Even if the test harness doesn't capture the output, and the patch uses System.console() to write directly to the console, that won't help testing, b/c then there's no way to validate what is written to the console...

MEXEC-118 on the other hand looks very testable, only it's a Windows specific feature, and I assume the release builds are done on Linux?  In any case, I will write a test for it and put it under a profile activated by <family>windows</family>.

Thanks,
Trask



On Sat, Jan 4, 2014 at 3:08 PM, Robert Scholte <[hidden email]> wrote:
Hi,

thanks for the patches.
A few remarks: Even though JDK6 has reached EOL, Mavens requirement is still JDK5.
So for MEXEC-121 you need to check if java.io.Console is available, since it was introduced with Java6

In both cases it would be nice to add an integration test, based on the maven-invoker-plugin[1], to confirm your solution.
There are already several tests under src/it
Run 'mvn verify' to confirm that everything still works.

thanks,
Robert

[1] http://maven.apache.org/plugins/maven-invoker-plugin/


Op Sat, 04 Jan 2014 23:54:59 +0100 schreef Trask Stalnaker <[hidden email]>:


Hi,

I have submitted 2 patches to the maven-exec-plugin project.  What's the
best way to help move these forward?

http://jira.codehaus.org/browse/MEXEC-118

http://jira.codehaus.org/browse/MEXEC-121

Thanks,
Trask

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

   http://xircles.codehaus.org/manage_email



Reply | Threaded
Open this post in threaded view
|

Re: 2 maven-exec-plugin patches

Robert Scholte-3
Hi Trask,

regarding MEXEC-121: after having a second look I think this looks okay. I  
only have some concerns for the WriterOutputStream. I think it should use  
the OS encoding. Since this piece of code is used by every IT it should  
already be covered. However, we could add a unittest to confirm that  
getConsoleOrSystemOut() returns System.out or not, based on the java  
version.

regarding MEXEC-121: no need to add a profile. If this test should only be  
executed with Windows, add a file called invoker.properties with entry
invoker.os.family = windows

See  
http://maven.apache.org/plugins/maven-invoker-plugin/run-mojo.html#invokerPropertiesFile

Releases are done on the local machine of the Release Manager (he who  
decided to do a release). In my case that would be Windows.

thanks,

Robert


Op Sun, 05 Jan 2014 18:30:12 +0100 schreef Trask Stalnaker  
<[hidden email]>:

> Hi Robert,
>
> Thanks for the response.
>
> The patch for MEXEC-121 already checks whether System.console() exists  
> and
> falls back to System.out if it does not (i.e. on JDK5).
>
> I looked at the test harness (invoker).  I'm not sure I can use it to  
> test
> MEXEC-121, since the test harness captures the output, and so
> System.console() will return null and the patch will fall back to using
> System.out in this case.  Even if the test harness doesn't capture the
> output, and the patch uses System.console() to write directly to the
> console, that won't help testing, b/c then there's no way to validate  
> what
> is written to the console...
>
> MEXEC-118 on the other hand looks very testable, only it's a Windows
> specific feature, and I assume the release builds are done on Linux?  In
> any case, I will write a test for it and put it under a profile activated
> by <family>windows</family>.
>
> Thanks,
> Trask
>
>
>
> On Sat, Jan 4, 2014 at 3:08 PM, Robert Scholte
> <[hidden email]>wrote:
>
>> Hi,
>>
>> thanks for the patches.
>> A few remarks: Even though JDK6 has reached EOL, Mavens requirement is
>> still JDK5.
>> So for MEXEC-121 you need to check if java.io.Console is available,  
>> since
>> it was introduced with Java6
>>
>> In both cases it would be nice to add an integration test, based on the
>> maven-invoker-plugin[1], to confirm your solution.
>> There are already several tests under src/it
>> Run 'mvn verify' to confirm that everything still works.
>>
>> thanks,
>> Robert
>>
>> [1] http://maven.apache.org/plugins/maven-invoker-plugin/
>>
>>
>> Op Sat, 04 Jan 2014 23:54:59 +0100 schreef Trask Stalnaker <
>> [hidden email]>:
>>
>>
>>  Hi,
>>>
>>> I have submitted 2 patches to the maven-exec-plugin project.  What's  
>>> the
>>> best way to help move these forward?
>>>
>>> http://jira.codehaus.org/browse/MEXEC-118
>>>
>>> http://jira.codehaus.org/browse/MEXEC-121
>>>
>>> Thanks,
>>> Trask
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe from this list, please visit:
>>
>>    http://xircles.codehaus.org/manage_email
>>
>>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email


Reply | Threaded
Open this post in threaded view
|

Re: 2 maven-exec-plugin patches

Trask Stalnaker
Hi Robert,

MEXEC-121:  I agree about using the OS encoding by default.  I updated the patch and made this the default, with a new property 'executableOutputCharset' to override it.  I also added an integration test.  Since this patch only has an effect when outputting to a console, the test doesn't prove anything when run as part of the invoker test suite, which is too bad, but if you run the test pom.xml from the command line (after manually changing @project.version@ in its pom.xml), you can see the effect of specifying the correct charset, which is nice at least.

MEXEC-118:  Integration test attached to issue.

Thanks again.

Trask



On Sun, Jan 5, 2014 at 10:13 AM, Robert Scholte <[hidden email]> wrote:
Hi Trask,

regarding MEXEC-121: after having a second look I think this looks okay. I only have some concerns for the WriterOutputStream. I think it should use the OS encoding. Since this piece of code is used by every IT it should already be covered. However, we could add a unittest to confirm that getConsoleOrSystemOut() returns System.out or not, based on the java version.

regarding MEXEC-121: no need to add a profile. If this test should only be executed with Windows, add a file called invoker.properties with entry
invoker.os.family = windows

See http://maven.apache.org/plugins/maven-invoker-plugin/run-mojo.html#invokerPropertiesFile

Releases are done on the local machine of the Release Manager (he who decided to do a release). In my case that would be Windows.

thanks,

Robert


Op Sun, 05 Jan 2014 18:30:12 +0100 schreef Trask Stalnaker <[hidden email]>:


Hi Robert,

Thanks for the response.

The patch for MEXEC-121 already checks whether System.console() exists and
falls back to System.out if it does not (i.e. on JDK5).

I looked at the test harness (invoker).  I'm not sure I can use it to test
MEXEC-121, since the test harness captures the output, and so
System.console() will return null and the patch will fall back to using
System.out in this case.  Even if the test harness doesn't capture the
output, and the patch uses System.console() to write directly to the
console, that won't help testing, b/c then there's no way to validate what
is written to the console...

MEXEC-118 on the other hand looks very testable, only it's a Windows
specific feature, and I assume the release builds are done on Linux?  In
any case, I will write a test for it and put it under a profile activated
by <family>windows</family>.

Thanks,
Trask



On Sat, Jan 4, 2014 at 3:08 PM, Robert Scholte
<[hidden email]>wrote:

Hi,

thanks for the patches.
A few remarks: Even though JDK6 has reached EOL, Mavens requirement is
still JDK5.
So for MEXEC-121 you need to check if java.io.Console is available, since
it was introduced with Java6

In both cases it would be nice to add an integration test, based on the
maven-invoker-plugin[1], to confirm your solution.
There are already several tests under src/it
Run 'mvn verify' to confirm that everything still works.

thanks,
Robert

[1] http://maven.apache.org/plugins/maven-invoker-plugin/


Op Sat, 04 Jan 2014 23:54:59 +0100 schreef Trask Stalnaker <
[hidden email]>:


 Hi,

I have submitted 2 patches to the maven-exec-plugin project.  What's the
best way to help move these forward?

http://jira.codehaus.org/browse/MEXEC-118

http://jira.codehaus.org/browse/MEXEC-121

Thanks,
Trask


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

   http://xircles.codehaus.org/manage_email



---------------------------------------------------------------------
To unsubscribe from this list, please visit:

   http://xircles.codehaus.org/manage_email