Bugzilla – Full Text Bug Listing
| Summary: | tycho-versions-plugin doesn't update product for eclipse-repository packaging | ||||||
|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Matthias Köster <matthias.koester> | ||||
| Component: | Tycho | Assignee: | Jan Sievers <jan.sievers> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P2 | CC: | allan.beharry, christian.georgi, cvgaviao, david_williams, d_a_carver, jan.sievers, julien.henry, lars.martin, oliver, pch, sbouchet, t-oberlies, youngm | ||||
| Version: | unspecified | ||||||
| Target Milestone: | --- | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Matthias Köster
This is in fact a bug. It would be great if you could provide a patch so that the versions in product files are also updated by the tycho-versions-plugin. Please attach the proposed patch to this bug report. Some background info to keep in mind: There is a conceptional problem in eclipse-repository - there can be more than one .product file, so unlike in eclipse-plugin and eclipse-feature, Tycho cannot enforce matching artifactId and version between the POM and the Eclipse artifact in eclipse-repository. This is why we have plans to move the product files to a new packaging type eclipse-product (bug 348586). Still, we will need the same version update functionality for eclipse-product, so if we had it for the current eclipse-repository, it will be easy to port this later on. Created attachment 205770 [details]
Support for versioning products in eclipse-repository
This patch implements version updates for the packaging type eclipse-repository. It updates all product files contained in the project. I also added a test for it.
The new class ProductFilesManipulator seems to have most code copied from ProductFileManipulator. Copy&paste code is a nightmare to maintain, so I cannot accept this patch. Please refactor the existing code to make it reusable. Hi Tobias, In your comment #1 you said that you're going to drop packaging type "eclipse-application" and because of that I decided to copy and paste the code. And I even showed that my patch works with a test case! If you're afraid of the maintenance nightmare caused by one additional class contributed to fix a bug that prevents other users from using your product, then I'm not able to contribute to your project! I'm using your product for our internal product and even did a very positive presentation about tycho at our local eclipse DemoCamp, but your answer really disappoints me. If you really want more contributions, you should provide more constructive feedback and write it in a more objective fashion. Calling a well written (ok, copy&pasted;-) patch a "nightmare to maintain" is IMHO not objective. A disappointed potential contributor. (In reply to comment #3) > The new class ProductFilesManipulator seems to have most code copied from > ProductFileManipulator. Copy&paste code is a nightmare to maintain, so I cannot > accept this patch. Please refactor the existing code to make it reusable. (In reply to comment #4) > In your comment #1 you said that you're going to drop packaging type > "eclipse-application" and because of that I decided to copy and paste the code. This is in fact planned, but with the very limited resources we have for Tycho this may still take a while. > Calling a well > written (ok, copy&pasted;-) patch a "nightmare to maintain" is IMHO not > objective. I wasn't referring to your patch in particular but rather to copy&paste code in general. I have suffered enough from copy&paste code, so my tolerance for it is low. This probably didn't come across in my last comment, but I do appreciate your contribution - the automated tests (and manual test which you certainly also did) a very useful. Also I understand that this is an urgent issue for you - so urgent that you have even taken the effort to even provide a patch. Therefore I think that we should have this fixed for the next release. Refactorings tend to be hard to review, so I guess it would be the easiest if I do the proposed changes to avoid the need for copy&paste. Hi Tobias, Thanks for you explanation what you meant, written communication with anonymous people has its own challenges from time to time;-) I hate copy and paste code too and always encourage people to refactor their code instead. But as you saw I'm fallible to that habit too ;-) Your suggestion sounds like a good solution to me, thanks for helping us. Looking forward to the next version of tycho, Matthias (In reply to comment #5) > (In reply to comment #4) > > In your comment #1 you said that you're going to drop packaging type > > "eclipse-application" and because of that I decided to copy and paste the code. > This is in fact planned, but with the very limited resources we have for Tycho > this may still take a while. > > > Calling a well > > written (ok, copy&pasted;-) patch a "nightmare to maintain" is IMHO not > > objective. > I wasn't referring to your patch in particular but rather to copy&paste code in > general. I have suffered enough from copy&paste code, so my tolerance for it is > low. > > This probably didn't come across in my last comment, but I do appreciate your > contribution - the automated tests (and manual test which you certainly also > did) a very useful. Also I understand that this is an urgent issue for you - so > urgent that you have even taken the effort to even provide a patch. Therefore I > think that we should have this fixed for the next release. > > Refactorings tend to be hard to review, so I guess it would be the easiest if I > do the proposed changes to avoid the need for copy&paste. @Matthias: I have moved the eclipse-application specific parts out of ProductFileManipulator (commit 74fa3cc) so that the new manipulator for eclipse-repository can use it as base class. Could you please provide an updated patch? The reason why I don't just apply your patch myself is because I anyway need you to answer the following questions: - Did you write the patch from scratch (i.e. without copying anything from outside the Tycho sources)? - Who owns the copyright of the patch? - Do you have the permission to contribute the code under the EPL? Please check the copyright header in ProductFilesManipulator - it seems that Sonatype doesn't own the copyright to that file, especially when using the new base class. Hi Tobias, Is there a documentation of the steps required to get a legal approvement to submit patches? Of course I have to get approval by my boss, but it would be easier if I know where to find all required steps beforehand ;-) And of course I would like to help you and get my patch in asap. Cheers, Matthias (In reply to comment #7) > @Matthias: I have moved the eclipse-application specific parts out of > ProductFileManipulator (commit 74fa3cc) so that the new manipulator for > eclipse-repository can use it as base class. Could you please provide an > updated patch? > > The reason why I don't just apply your patch myself is because I anyway need > you to answer the following questions: > - Did you write the patch from scratch (i.e. without copying anything from > outside the Tycho sources)? > - Who owns the copyright of the patch? > - Do you have the permission to contribute the code under the EPL? > > Please check the copyright header in ProductFilesManipulator - it seems that > Sonatype doesn't own the copyright to that file, especially when using the new > base class. I'll probably need to file a CQ for the patch anyway, so we can have the Eclipse IP team answer the question. @Mattias: Sorry if I wasn't clear on this: In order to trigger further activities for including your patch, I need an updated version of the patch which fixes the copyright statement in ProductFilesManipulator. For this, I'd recommend to also rebase the patch onto 74fa3cc (e.g. master). Then that file probably only contains code you have written - and this makes the copyright statement easier. You can find the template for an EPL copyright header here: http://www.eclipse.org/legal/copyrightandlicensenotice.php @Tobias: I see, I'll try to provide you with a patch over the weekend. (In reply to comment #10) > @Mattias: Sorry if I wasn't clear on this: In order to trigger further > activities for including your patch, I need an updated version of the patch > which fixes the copyright statement in ProductFilesManipulator. > > For this, I'd recommend to also rebase the patch onto 74fa3cc (e.g. master). > Then that file probably only contains code you have written - and this makes > the copyright statement easier. You can find the template for an EPL copyright > header here: http://www.eclipse.org/legal/copyrightandlicensenotice.php This is stalled due to missing IP clearance; resetting target milestone field. Is there any progress concerning this one? Again: any progress here? Any hints on how to proceed are welcome. Note: The versions-plugin will only update the versions in product files, if the old version matches the version in the eclipse-repository POM. Product files whose versions are (intentionally) different from the POM version are not updated. This is the same as with modules: modules which don't have the same version as the root POM are not updated. Sounds like a reasonable solution. My product files match the version in the POM. Thanks. |