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
<trask.stalna...@gmail.com>:
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
<codeh...@sourcegrounds.com>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 <
trask.stalna...@gmail.com>:
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
