You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/07/24 07:27:55 UTC
[GitHub] [maven-shared-utils] nywitness opened a new pull request #60: [MSHARED-938]
nywitness opened a new pull request #60:
URL: https://github.com/apache/maven-shared-utils/pull/60
add charset config
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] michael-o commented on a change in pull request #60: [MSHARED-938]
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #60:
URL: https://github.com/apache/maven-shared-utils/pull/60#discussion_r462920199
##########
File path: src/test/java/org/apache/maven/shared/utils/cli/CommandLineUtilsTest.java
##########
@@ -168,4 +169,25 @@ private void assertCmdLineArgs( final String[] expected, final String cmdLine )
assertEquals( expected.length, actual.length );
assertEquals( Arrays.asList( expected ), Arrays.asList( actual ) );
}
+
+ @Test
+ public void testChineseEncodingIssue()
Review comment:
That's wrong. You need to use System.out as an byte stream, not a char stream: `byte[] bytes = "...".getBytes(encoding)` then `System.out.write(bytes)`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] michael-o commented on a change in pull request #60: [MSHARED-938]
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #60:
URL: https://github.com/apache/maven-shared-utils/pull/60#discussion_r461593712
##########
File path: src/test/java/org/apache/maven/shared/utils/cli/CommandLineUtilsTest.java
##########
@@ -168,4 +169,25 @@ private void assertCmdLineArgs( final String[] expected, final String cmdLine )
assertEquals( expected.length, actual.length );
assertEquals( Arrays.asList( expected ), Arrays.asList( actual ) );
}
+
+ @Test
+ public void testChineseEncodingIssue()
+ throws Exception
+ {
+ Commandline commandline = new Commandline( "ping www.baidu.com" );
Review comment:
No, we cannot have tests which require outbound access. Espcially ICMP. Please change test.
##########
File path: src/main/java/org/apache/maven/shared/utils/cli/CommandLineUtils.java
##########
@@ -280,11 +280,11 @@ public Integer call()
inputFeeder.start();
}
- outputPumper = new StreamPumper( p.getInputStream(), systemOut );
+ outputPumper = new StreamPumper( p.getInputStream(), systemOut , streamCharset );
Review comment:
Space before comma is not required.
##########
File path: src/test/java/org/apache/maven/shared/utils/cli/CommandLineUtilsTest.java
##########
@@ -168,4 +169,25 @@ private void assertCmdLineArgs( final String[] expected, final String cmdLine )
assertEquals( expected.length, actual.length );
assertEquals( Arrays.asList( expected ), Arrays.asList( actual ) );
}
+
+ @Test
+ public void testChineseEncodingIssue()
Review comment:
This test is completely pointless:
1. The `echo` will use the encoding supplied the entire system. There is no guarantee that GBK is the system encoding.
2. You never verify the output of th command to be what you expect.
What you require is an application that wroduces GBK *bytes* , those are read by Java with GBK into a `String` then you need to compared this value.
##########
File path: src/test/java/org/apache/maven/shared/utils/cli/CommandLineUtilsTest.java
##########
@@ -168,4 +169,25 @@ private void assertCmdLineArgs( final String[] expected, final String cmdLine )
assertEquals( expected.length, actual.length );
assertEquals( Arrays.asList( expected ), Arrays.asList( actual ) );
}
+
+ @Test
+ public void testChineseEncodingIssue()
Review comment:
This test is completely pointless:
1. The `echo` will use the encoding supplied by the entire system. There is no guarantee that GBK is the system encoding.
2. You never verify the output of th command to be what you expect.
What you need is an application that produces GBK *bytes* , those are read by Java with GBK into a `String` then you need to compare this value.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] michael-o commented on a change in pull request #60: [MSHARED-938]
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #60:
URL: https://github.com/apache/maven-shared-utils/pull/60#discussion_r462164339
##########
File path: src/test/java/org/apache/maven/shared/utils/cli/CommandLineUtilsTest.java
##########
@@ -168,4 +169,25 @@ private void assertCmdLineArgs( final String[] expected, final String cmdLine )
assertEquals( expected.length, actual.length );
assertEquals( Arrays.asList( expected ), Arrays.asList( actual ) );
}
+
+ @Test
+ public void testChineseEncodingIssue()
Review comment:
> As far as i am concerned, the parameter commandLine of CommandLineUtils.executeCommandLineAsCallable is used to create a Process object, which is a result of Runtime.getRuntime().execute(). This execute() method uses different encoding depending on different system.
Why do you think so? It uses the same encoding as the surrounding Java process does. You cannot change this really on Windows, on Unix you can pass LC_ALL to the env.
> Any idea of producing GBK bytes using CommandLineUtils.executeCommandLineAsCallable? Or i can modify the test to use system encoding rather than using GBK.
You have two options:
1. Modify `file.encoding` and set back in the finally block. Implies you read the output stream. I don't exactly know whether tests can run in parallel in the same JVM, this could break other tests.
2. Write a simple Java program, put it in `src/test/java`, call the `.class` file with Java from within the test. It should use `System.out` as a byte-oriented stream which will write bytes according to GBK. Read those with the consumer and check when normalized back to UTF-16.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] nywitness commented on pull request #60: [MSHARED-938]
Posted by GitBox <gi...@apache.org>.
nywitness commented on pull request #60:
URL: https://github.com/apache/maven-shared-utils/pull/60#issuecomment-664713924
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] nywitness commented on a change in pull request #60: [MSHARED-938]
Posted by GitBox <gi...@apache.org>.
nywitness commented on a change in pull request #60:
URL: https://github.com/apache/maven-shared-utils/pull/60#discussion_r462750423
##########
File path: src/test/java/org/apache/maven/shared/utils/cli/CommandLineUtilsTest.java
##########
@@ -168,4 +169,25 @@ private void assertCmdLineArgs( final String[] expected, final String cmdLine )
assertEquals( expected.length, actual.length );
assertEquals( Arrays.asList( expected ), Arrays.asList( actual ) );
}
+
+ @Test
+ public void testChineseEncodingIssue()
Review comment:
> Write a simple Java program, put it in src/test/java, call the .class file with Java from within the test. It should use System.out as a byte-oriented stream which will write bytes according to GBK. Read those with the consumer and check when normalized back to UTF-16.
I tried producing gbk bytes with `System.out.println(new String("金色传说".getBytes(), "GBK")`. When comparing value in the cousumer, test passes on a windows-gbk platform but fails on a mac-utf-8 platform. Maybe it's not right to produce gbk bytes like that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] nywitness commented on a change in pull request #60: [MSHARED-938]
Posted by GitBox <gi...@apache.org>.
nywitness commented on a change in pull request #60:
URL: https://github.com/apache/maven-shared-utils/pull/60#discussion_r462115011
##########
File path: src/test/java/org/apache/maven/shared/utils/cli/CommandLineUtilsTest.java
##########
@@ -168,4 +169,25 @@ private void assertCmdLineArgs( final String[] expected, final String cmdLine )
assertEquals( expected.length, actual.length );
assertEquals( Arrays.asList( expected ), Arrays.asList( actual ) );
}
+
+ @Test
+ public void testChineseEncodingIssue()
Review comment:
By the way, this test will pass without this PR if system encoding is the same as the result of `Character.defaultCharset()`. So the influence of this fix may not be very obvious.
##########
File path: src/test/java/org/apache/maven/shared/utils/cli/CommandLineUtilsTest.java
##########
@@ -168,4 +169,25 @@ private void assertCmdLineArgs( final String[] expected, final String cmdLine )
assertEquals( expected.length, actual.length );
assertEquals( Arrays.asList( expected ), Arrays.asList( actual ) );
}
+
+ @Test
+ public void testChineseEncodingIssue()
Review comment:
By the way, this test will pass without this PR if system encoding is the same as the result of `Charset.defaultCharset()`. So the influence of this fix may not be very obvious.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org