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 <codeh...@sourcegrounds.com>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 < > 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 > > >
