Bugzilla – Full Text Bug Listing
| Summary: | File encoding configuration. | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Olivier NOUGUIER <olivier.nouguier> | ||||||||||||||
| Component: | m2e | Assignee: | Project Inbox <m2e.core-inbox> | ||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||
| Severity: | enhancement | ||||||||||||||||
| Priority: | P3 | CC: | anders.g.hammar, fbricon, igor | ||||||||||||||
| Version: | unspecified | ||||||||||||||||
| Target Milestone: | --- | ||||||||||||||||
| Hardware: | PC | ||||||||||||||||
| OS: | Mac OS X - Carbon (unsup.) | ||||||||||||||||
| Whiteboard: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Olivier NOUGUIER
Created attachment 191575 [details]
Patch implementing the enhancement
Created attachment 191857 [details]
test project
Test: for org.eclipse.m2e.tests
I think the patch needs more work... First, I do not believe it is appropriate to set encoding for entire project based on maven-compiler-plugin configuration. It is better to set encoding for java source folders only. Second, I believe IContainer.setDefaultCharset is the right way to set default encoding for all files in a project or folder. Suggested Platform.getPreferencesService relies on non-API internal implementation of org.eclipse.core.resources bundle. Third, changing addJavaProjectOptions to return encoding does not make a good (internal) API. I think it is more appropriate to have deal with encoding as part of addProjectSourceFolders logic, or at least have separate getDefaultCharset call. Created attachment 192588 [details]
Enhancement corrected.
Following your wise advices ... sorry for the poor first attempt!
The new version of the patch look good. Can you please provide an automated regression test? m2e tests currently live in https://github.com/sonatype/m2e-core-tests and we need git-format-patch formatted patch. Created attachment 192673 [details]
Test case
I like to but an:
cd org.eclipse.m2e.tests
mvn clean integration-test
lead to:
[ERROR] Failed to execute goal org.sonatype.tycho:maven-osgi-compiler-plugin:0.10.0:compile (default-compile) on project org.eclipse.m2e.tests: Compilation failure: Compilation failure:
[ERROR] /Users/chelebithil/works/sonatype/m2e-core-tests/org.eclipse.m2e.tests/src/org/eclipse/m2e/tests/AddMarkersProjectConfiguratorFoo.java (at line 16):[-1,-1]
[ERROR] import org.eclipse.m2e.core.lifecyclemapping.model.IPluginExecutionMetadata;
...
...
...
If I remove the guilty files: (org/eclipse/m2e/tests/ProjectConfigurationManagerTest.java for example) then my test passes and seem ok.
Is it the right place to put those test?
Is there a reason why those test are failing (even not compiling) on my platform/install?
Thanks.
(In reply to comment #6) > Is it the right place to put those test? > Is there a reason why those test are failing (even not compiling) on my > platform/install? > Yes, this is the right place. Most likely tests used older/newer m2e snapshot. We are refactoring m2e public APIs right now, so this kinda expected. Few comments about the test case itself * Can you use more exotic <encoding/> value? All our CI systems use UTF-8, so the test will pass on them with or without the fix. * Can you extend the test to verify change of <encoding/> value? You can use one of AbstractMavenProjectTestCase.copyContent methods to simulate changes to pom.xml file and invoke MavenPlugin.getDefault().getProjectConfigurationManager().updateProjectConfiguration to force project configuration update from the test * m2e test fixture only has access to small number of maven plugins. You need to use <parent/> element like below to make sure the test project uses available versions <parent> <groupId>org.eclipse.m2e.test</groupId> <artifactId>m2e-test-parent</artifactId> <version>1.0.0</version> </parent> Also, I was not able to make the test pass or fail on purpose using Eclipse 3.7M6 target platform on Linux. What eclipse and os do you use? > Also, I was not able to make the test pass or fail on purpose using Eclipse
> 3.7M6 target platform on Linux. What eclipse and os do you use?
3.7M5 / MacOsX
I will switch to M6 tonight.
Created attachment 192791 [details]
Test case
The configurator was not called, because the test project had no "src/main/java" folder...
Now I use ISO-8859-1 and UTF-16 as test values.
The patch and the tests look good now. Applied to pushed (I did update test pom.xml files to use proper parent and compile-plugin version, hope you don't mind). Thank you for the patch and for your patience. http://git.eclipse.org/c/m2e/m2e-core.git/commit/?id=e5c47fda4396082ec2f7fd93ded3d6f2218eccf6 https://github.com/sonatype/m2e-core-tests/commit/9bbf3f076439c4e97841189e42d23b548efc8797 Now lets hope I got eclipse ip process right... (In reply to comment #10) > The patch and the tests look good now. Applied to pushed (I did update test > pom.xml files to use proper parent and compile-plugin version, hope you don't > mind). Thank you for the patch and for your patience. > > http://git.eclipse.org/c/m2e/m2e-core.git/commit/?id=e5c47fda4396082ec2f7fd93ded3d6f2218eccf6 > https://github.com/sonatype/m2e-core-tests/commit/9bbf3f076439c4e97841189e42d23b548efc8797 > > Now lets hope I got eclipse ip process right... Igor, thank you too for your guidance and you patience. Very nice feature for us living outside plain ASCII land! However, I've tested m2e 0.13.0.201104251605 and encoding is not configured for a plugin added source folder with generated sources. I tested with a Maven project with a binding to the modello plugin. (In reply to comment #12) > However, I've tested m2e 0.13.0.201104251605 and encoding is not configured for > a plugin added source folder with generated sources. I tested with a Maven > project with a binding to the modello plugin. I have the m2e connector for modello installed, which is adding this source folder to the Eclipse project. So it's that connector that's not setting the encoding on the folder. Created attachment 194269 [details]
Integration test showing bug in implementation
I think there is a bug in the current implementation. In the case where the defined encoding is removed, the folder encoding should revert to container default (which it currently doesn't). The attached git patch for the integration tests adds a test which shows this.
I believe the simple solution is to apply the encoding to the IFolder even when it's 'null', which means it's not defined in the Maven project.
(In reply to comment #14) > Created attachment 194269 [details] > Integration test showing bug in implementation > > I think there is a bug in the current implementation. In the case where the > defined encoding is removed, the folder encoding should revert to container > default (which it currently doesn't). The attached git patch for the > integration tests adds a test which shows this. > I believe the simple solution is to apply the encoding to the IFolder even when > it's 'null', which means it's not defined in the Maven project. Filed bug 344657 for this issue. Comment on attachment 192588 [details]
Enhancement corrected.
cleaning IP Log up
|