Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Wtp Component File Version #2076

Merged
merged 1 commit into from Jun 7, 2017

Conversation

Vampire
Copy link
Contributor

@Vampire Vampire commented May 18, 2017

The wtp component file that was generated out of nowhere (no file already present before running the task) generated a version of 2.0.
Current Eclipse Neon (seen this behavior also in Kepler) has a version of 1.5.0 if Eclipse generates this file.
Having 2.0 in this file triggers the bug (or behavior) described in https://bugs.eclipse.org/bugs/show_bug.cgi?id=416346.

  • Review Contribution Guidelines

  • Sign Gradle CLA

  • (n/a) Link to Design Spec for changes that affect more than 1 public API (that is, not in an internal package) or updates to > 20 files

  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective

  • (n/a) Provide unit tests (under <subproject>/src/test) to verify logic

  • (n/a) Update User Guide, DSL Reference, and Javadoc for public-facing changes

  • Ensure that tests pass locally: ./gradlew quickCheck <impacted-subproject>:check

  • Verify design and implementation

  • Verify test coverage and CI build status

  • Verify documentation including proper use of @since and @Incubating annotations for all public APIs

  • Recognize contributor in release notes

@oehme
Copy link
Contributor

oehme commented May 18, 2017

This code hasn't changed in 7 years, so it would be surprising if we suddenly needed to downgrade the version. I haven't seen the described behavior before. A reproducible example project would be very helpful in assessing this PR.

@Vampire
Copy link
Contributor Author

Vampire commented May 18, 2017

This is less a downgrade than a correction.
As the current verison as of Eclipse generated file is 1.5.0, why do you think 2.0 could be correct?

And just that the bug is very old, does not mean that it is not present.
It was about 4 years ago that I was hit by it, but attributed it to Eclipse.
It was just today that I by accident found out that this is actually a Gradle bug.

In the attached wst-bug.zip you find a good and a bad version of the same project files. The only difference is 2.0 in bad vs. 1.5.0 in good. In before you see them without Eclipse having touched them, in after you see them after Eclipse imported them. Only importing exising project into workspace, that's all.

You will see that besides whitespace and the version change one line was thrown out in the bad version.

That the bug was not discovered in the last 7 years can have multiple reasons. I don't know what exactly in Eclipse throws out this line and whether it is a bug or some "oh it is not 1.5.0, so it must be older, let's transform it" kind of thing is and what exactly is thrown out. Probably most people don't generate the files like that, but we have the web-part spread out into the business modules and the web project then pulls the pieces together like that. Also once you have imported the file in Eclipse and it has written 1.5.0 in the version field, as long as you don't delete the file or do cleanEclipse, the existing file will be updated which also means that the verison field remains untouched.

@Vampire
Copy link
Contributor Author

Vampire commented May 23, 2017

@oehme is the attached description and project enough to justify or is something missing?

@oehme
Copy link
Contributor

oehme commented Jun 7, 2017

@Vampire yes that is sufficient, thanks for taking the time to investigate. It would still be great if we could get some clarification on the exact meaning of the project-version field from a WTP team member. @mickaelistria do you know someone how could shed some light on this?

@mickaelistria
Copy link

Maybe Konstantin Komissarchik, but the best thing is to ask on the wtp-dev@eclipse.org mailing-list.

@Vampire
Copy link
Contributor Author

Vampire commented Jun 7, 2017

I had a quick look at the Eclipse sources.
You can find the relevant source at http://git.eclipse.org/c/webtools-common/webtools.common.git/tree/plugins/org.eclipse.wst.common.modulecore/modulecore-src/org/eclipse/wst/common/componentcore/internal/ModuleStructuralModel.java#n135.
There it checks whether the version is 1.5.0 with comment Check and see if we need to clean up spurrious redundant map entries and if not calls cleanupWTPModules with comment This method is used to remove spurrious redundant entries from the .component file.
Obviously that code is erroneous as the deployment of our generated file cleaned up by that method does not work.
The first version of this code that always does the cleanup was added 2006 with commit http://git.eclipse.org/c/webtools-common/webtools.common.git/commit/plugins/org.eclipse.wst.common.modulecore/modulecore-src/org/eclipse/wst/common/componentcore/internal/ModuleStructuralModel.java?id=eef9c4cdfd86906de3f4556c0e08f5310d30736c to fix bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=126291.
Also 2006 with commit http://git.eclipse.org/c/webtools-common/webtools.common.git/commit/plugins/org.eclipse.wst.common.modulecore/modulecore-src/org/eclipse/wst/common/componentcore/internal/ModuleStructuralModel.java?id=6d49acce7de0a06478f161031bf525adc43e8b90 it was introduced that this cleanup is only done if project-version is not equal to 1.5.0 and project version is set when doing the cleanup.

@oehme
Copy link
Contributor

oehme commented Jun 7, 2017

Okay, I'm sold 👍 Thanks a lot for all your analysis.

@oehme oehme changed the base branch from release to master June 7, 2017 13:45
@oehme oehme merged commit 2c473a7 into gradle:master Jun 7, 2017
@oehme oehme added this to the 4.1 M1 milestone Jun 7, 2017
@oehme oehme self-assigned this Jun 7, 2017
@Vampire Vampire deleted the fix-wtp-component-file-version branch June 7, 2017 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants