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 2022/02/24 19:28:56 UTC
[GitHub] [maven-plugin-tools] SebastianKuehn opened a new pull request #73: [MPLUGIN-394] Respect input encoding
SebastianKuehn opened a new pull request #73:
URL: https://github.com/apache/maven-plugin-tools/pull/73
PluginReport didn't specify an encoding while reading the
Plugin-Descriptor. This causes the fallback to the plattform encoding
and ignores the (user specified) input encoding.
This commits adds an integration test to reproduce the error and fixes
it by using the input encoding for the creation of the used Reader.
The integration tests forces the plattform encoding to CP1252 (Western
European charset used in Windows) while the source (input) and output
encoding is UTF-8.
The source file contains non-ASCII characters which have a different
representation in both encodings.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] slawekjaranowski commented on pull request #73: [MPLUGIN-394] Respect input encoding
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on pull request #73:
URL: https://github.com/apache/maven-plugin-tools/pull/73#issuecomment-1050197402
@SebastianKuehn please also look at #72
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] michael-o commented on a change in pull request #73: [MPLUGIN-394] Respect input encoding
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #73:
URL: https://github.com/apache/maven-plugin-tools/pull/73#discussion_r815426373
##########
File path: maven-plugin-plugin/src/it/mplugin-394_report-encoding/invoker.properties
##########
@@ -0,0 +1,18 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+invoker.goals = -Dfile.encoding=CP1252 plugin:report
Review comment:
I know and am I bit late, but this is brittle and wrong. This system property shall only be set, at most, at JVM startup time. I highly doubt that Invoker does that.
Read JEP 400:
Developers sometimes attempt to configure the default charset by setting the system property
file.encoding on the command line (i.e., java -Dfile.encoding=...), but this has never been supported.
Furthermore, attempting to set the property programmatically (i.e., System.setProperty(...)) after the
Java runtime has started does not work.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] michael-o commented on a change in pull request #73: [MPLUGIN-394] Respect input encoding
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #73:
URL: https://github.com/apache/maven-plugin-tools/pull/73#discussion_r824149087
##########
File path: maven-plugin-plugin/src/it/mplugin-394_report-encoding/invoker.properties
##########
@@ -0,0 +1,18 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+invoker.goals = -Dfile.encoding=CP1252 plugin:report
Review comment:
Not really, since there is no portable way. What you are doing is you are passing a user property, not a system property. Unfortunately, Maven promotes those to system properties which is wrong and will likely go away with 4 GA. If you can create a `jvm.config` and supply a README which describes this problem, I'd be happy with that, but in the `goals` it is logically wrong.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] hboutemy commented on a change in pull request #73: [MPLUGIN-394] Respect input encoding
Posted by GitBox <gi...@apache.org>.
hboutemy commented on a change in pull request #73:
URL: https://github.com/apache/maven-plugin-tools/pull/73#discussion_r824427199
##########
File path: maven-plugin-plugin/src/it/mplugin-394_report-encoding/invoker.properties
##########
@@ -0,0 +1,18 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+invoker.goals = -Dfile.encoding=CP1252 plugin:report
Review comment:
honestly, we don't really write ITs for such encoding issues: there is no chance of regression at plugin level, only base encoding-aware XML reading class is well tested
so IMHO, don't worry too much, eventually remove the IT that was done if it's not useful: it was done based on good principles, but life is life on such feature
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] SebastianKuehn commented on a change in pull request #73: [MPLUGIN-394] Respect input encoding
Posted by GitBox <gi...@apache.org>.
SebastianKuehn commented on a change in pull request #73:
URL: https://github.com/apache/maven-plugin-tools/pull/73#discussion_r822365765
##########
File path: maven-plugin-plugin/src/it/mplugin-394_report-encoding/invoker.properties
##########
@@ -0,0 +1,18 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+invoker.goals = -Dfile.encoding=CP1252 plugin:report
Review comment:
Hallo @michael-o,
this was my reproduction of the problem (on a Windows system which has CP1252 as System-Encoding). My goal was to set the default encoding to CP1252, regardless of the system (Linux, Mac etc.). In hindsight you are right, that this test might not work as expected an non Windows machine (i.e. it never fails because the encoding is always UTF-8). Can you propose another way to safely accomplish a default/system/native encoding not equal to utf8 while running this integration test?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] michael-o commented on a change in pull request #73: [MPLUGIN-394] Respect input encoding
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #73:
URL: https://github.com/apache/maven-plugin-tools/pull/73#discussion_r824972534
##########
File path: maven-plugin-plugin/src/it/mplugin-394_report-encoding/invoker.properties
##########
@@ -0,0 +1,18 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+invoker.goals = -Dfile.encoding=CP1252 plugin:report
Review comment:
@SebastianKuehn Please add a followup PR which documents the limitations of this PR
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] SebastianKuehn commented on pull request #73: [MPLUGIN-394] Respect input encoding
Posted by GitBox <gi...@apache.org>.
SebastianKuehn commented on pull request #73:
URL: https://github.com/apache/maven-plugin-tools/pull/73#issuecomment-1050214668
@hboutemy, @slawekjaranowski I added an commit to use `XMLStreamReader` to detect the encoding from the input xml.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] hboutemy commented on pull request #73: [MPLUGIN-394] Respect input encoding
Posted by GitBox <gi...@apache.org>.
hboutemy commented on pull request #73:
URL: https://github.com/apache/maven-plugin-tools/pull/73#issuecomment-1053514698
great job, thank you @SebastianKuehn
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] hboutemy merged pull request #73: [MPLUGIN-394] Respect input encoding
Posted by GitBox <gi...@apache.org>.
hboutemy merged pull request #73:
URL: https://github.com/apache/maven-plugin-tools/pull/73
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org