Bugzilla – Full Text Bug Listing
| Summary: | [patch] Provide a way to build the documentation index | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Marc-André Laperle <malaperle> | ||||||
| Component: | Tycho | Assignee: | Jan Sievers <jan.sievers> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | enhancement | ||||||||
| Priority: | P3 | CC: | igor, jan.sievers, krzysztof.daniel, mober.at+eclipse | ||||||
| Version: | unspecified | ||||||||
| Target Milestone: | --- | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 350861, 350828, 369982 | ||||||||
| Attachments: |
|
||||||||
|
Description
Marc-André Laperle
Created attachment 208218 [details]
BuildHelpIndexMojo patch
The patch looks okay, but I have couple of suggestions 1. Apart of very few help-specific constants, the patch essentially introduces a generic mechanism to dynamically resolve and execute eclipse applications. I wonder if it makes sense to refactor the code further to make application root IUs and execution parameters fully configurable. We can call this tycho-eclipserun-plugin. 2. I do not think help index generation belongs to the main Tycho source tree, I think tycho-extras is more appropriate. Even tycho-eclipserun-plugin is probably better suited for tycho-extras. Also, as explained in [1], we prefer git-format-patch patches. [1] http://wiki.eclipse.org/Tycho/Contributor_Guide Created attachment 208241 [details] EclipseRunMojo patch (In reply to comment #2) > The patch looks okay, but I have couple of suggestions Thank you for the prompt feedback! > 1. Apart of very few help-specific constants, the patch essentially introduces > a generic mechanism to dynamically resolve and execute eclipse applications. I > wonder if it makes sense to refactor the code further to make application root > IUs and execution parameters fully configurable. We can call this > tycho-eclipserun-plugin. I took out the hard coded dependencies that had to do with the help, now they have to be specified in the plugin configuration. Also, like TestMojo, VM arguments, program arguments and environment variables can now be specified for greater flexibility. I don't know what you mean by making application root IUs (sorry what's an IU?), do you mean to make it an API? > 2. I do not think help index generation belongs to the main Tycho source tree, > I think tycho-extras is more appropriate. Even tycho-eclipserun-plugin is > probably better suited for tycho-extras. That makes sense. I renamed the plugin to tycho-eclipserun-plugin and moved it to tycho-extras, renamed the Mojo to EclipseRunMojo and the goal is now eclipse-run. > Also, as explained in [1], we prefer git-format-patch patches. > > [1] http://wiki.eclipse.org/Tycho/Contributor_Guide Sorry about that, I tried git format-patch but it wasn't giving me any output and I was hoping git-gui would give similar result. I figured out what I was doing wrong so this new patch should be in the right format. BTW, I think I read somewhere that Tycho 0.14 only works with Juno, is that right? I was hoping we could use this new plugin to build the documentation index for CDT in Indigo SR2. Thank you for your help! (In reply to comment #3) > BTW, I think I read somewhere that Tycho 0.14 only works with Juno, is that > right? I was hoping we could use this new plugin to build the documentation > index for CDT in Indigo SR2. Since the EclipseRunMojo is now a plugin (in tycho-extras), could it perhaps also run with older versions of Tycho ? And be provisioned as a jar separate from main Tycho ? Comment on attachment 208241 [details] EclipseRunMojo patch The patch is <250 LOC, so I believe CQ is not required (http://eclipse.org/legal/EclipseLegalProcessPoster.pdf) Applied the patch, thank you. If you have time, it would be great if you could write a wiki somewhere under [1] that explains how to use eclipserun plugin to generate help index [1] http://wiki.eclipse.org/Category:Tycho (In reply to comment #3) > > I don't know what you mean by making application root IUs (sorry what's an > IU?), do you mean to make it an API? > "IU" stands for "(p2) Installable Unit", this is what Tycho uses internally to represent <dependency> configuration elements. > > BTW, I think I read somewhere that Tycho 0.14 only works with Juno, is that > right? I was hoping we could use this new plugin to build the documentation > index for CDT in Indigo SR2. We test Tycho with Tycho eclipse 3.7 and 3.8/4.2 (and probably with 3.6 as well). I am not sure if anyone is testing with earlier Eclipse versions, but generally all versions starting with eclipse 3.4 are expected to "just work" and it should be possible to make 3.0-3.3 work with additional setup. (In reply to comment #4) > > BTW, I think I read somewhere that Tycho 0.14 only works with Juno, is that > > right? I was hoping we could use this new plugin to build the documentation > > index for CDT in Indigo SR2. > > Since the EclipseRunMojo is now a plugin (in tycho-extras), could it perhaps > also run with older versions of Tycho ? And be provisioned as a jar separate > from main Tycho ? There are API changes between 0.14 and earlier versions of Tycho. Although it should not be hard to backport eclipserun plugin to an earlier Tycho version, there are currently no plans to do this. (In reply to comment #6) > Applied the patch, thank you. If you have time, it would be great if you could > write a wiki somewhere under [1] that explains how to use eclipserun plugin to > generate help index > > [1] http://wiki.eclipse.org/Category:Tycho I added a section in [1], let me know if anything needs to be changed or added. Thank you for adding this feature to Tycho! [1] http://wiki.eclipse.org/Tycho/Additional_Tools (In reply to comment #7) > I added a section in [1], let me know if anything needs to be changed or added. > Thank you for adding this feature to Tycho! > > [1] http://wiki.eclipse.org/Tycho/Additional_Tools This looks very good :-) If you want, you can also advertise (i.e. with a short summary + link) the feature on the release notes page for the next release [2] [2] http://wiki.eclipse.org/Tycho/Release_Notes/0.14 We need to introduce at least most basic integration test to cover functionality provided by eclipse-run mojo from regressions. Also, I am not convinced that using project dependencies is expected/desired here. Original usecase for eclipse-run mojo was to build documentation index, which most likely does not require project dependencies, but some other unrelated set of bundles. Can somebody familiar with the original usecase comment on this? (In reply to comment #9) > We need to introduce at least most basic integration test to cover > functionality provided by eclipse-run mojo from regressions. Will do! (In reply to comment #10) > Also, I am not convinced that using project dependencies is expected/desired > here. Original usecase for eclipse-run mojo was to build documentation index, > which most likely does not require project dependencies, but some other > unrelated set of bundles. > > Can somebody familiar with the original usecase comment on this? It would be great to remove the need to specify dependencies. - org.eclipse.help.base : This is needed because inside the ant build file, we execute the help.buildHelpIndex Ant task. I don't see a good way of getting around that unless we want to parse the ant file, build a list of tasks then search for the projects that contain them. That seems out of the scope of this plugin. - org.apache.ant : This is needed because it seems like jars.extra.classpath = platform:/plugin/org.apache.ant/lib/ant.jar in org.eclipse.help.base/build.properties is not taken into account when resolving the dependencies. Maybe this could be improved? move all open bugs to post 0.14.0 This bug has a working patch attached, which is also marked iplog+ so I assume it has been committed ? What is the rationale behind moving this to post 0.14 ? For a contributor, it is always frustrating when a contribution is not at least commented on and dealt with. Looking at the last interactions, it seemed to me - Marc-Andre has an open action to contribute a basic integration test ? Which seems to me could be building the help index for an actual plugin? - The other question about project dependencies has been answered by Marc-Andre, the two dependencies seem to be in order from my point of view so there is no real blocker to accepting the contribution ? (In reply to comment #13) > This bug has a working patch attached, which is also marked iplog+ so I assume > it has been committed ? > > What is the rationale behind moving this to post 0.14 ? > > For a contributor, it is always frustrating when a contribution is not at least > commented on and dealt with. Looking at the last interactions, it seemed to me > > - Marc-Andre has an open action to contribute a basic integration test ? > Which seems to me could be building the help index for an actual plugin? > The code has been committed to tycho-extras repository but we can't really tell if it works or not without an IT. Even if it does work, we can't promise it will continue to work. (In reply to comment #14) > The code has been committed to tycho-extras repository but we can't really > tell if it works or not without an IT. Even if it does work, we can't > promise it will continue to work. IT proposed https://git.eclipse.org/r/#/c/9626/ we can't afford taking contributions without tests anymore. Updated http://wiki.eclipse.org/Tycho/Contributor_Guide#Developing_Patches_for_Tycho accordingly. (In reply to comment #15) > (In reply to comment #14) > > The code has been committed to tycho-extras repository but we can't really > > tell if it works or not without an IT. Even if it does work, we can't > > promise it will continue to work. > > IT proposed > https://git.eclipse.org/r/#/c/9626/ > > we can't afford taking contributions without tests anymore. Updated > http://wiki.eclipse.org/Tycho/Contributor_Guide#Developing_Patches_for_Tycho > accordingly. I have a IT in progress but I haven't had the time to finish it yet. The CDT project has been using it for several releases so at least we know it works for this use case. as I said, I wrote an IT now covering the basic eclipse antrunner scenario about one year after the fact because I needed to touch the code for bug 395281. The point that Igor was trying to make earlier is that it's not sufficient to "know the code works" but having no test that tells you so in order to 1. keep it working while everything around it changes 2. be able to change/evolve it BTW with bug 395281 there will be an incompatible change (explicit p2 repo configuration independent of target platform required for this plugin), see https://git.eclipse.org/r/#/c/9625/ we will announce the change with instructions how to migrate in the release notes http://wiki.eclipse.org/Tycho/Release_Notes/0.17 (In reply to comment #17) > as I said, I wrote an IT now covering the basic eclipse antrunner scenario > about one year after the fact because I needed to touch the code for bug > 395281. Thanks, sorry about that. > The point that Igor was trying to make earlier is that it's not sufficient > to "know the code works" but having no test that tells you so in order to > > 1. keep it working while everything around it changes > 2. be able to change/evolve it I agree, I actually didn't expect it do be accepted without a test. The test I wrote is all Java, similar to other mojos and it builds the documentation index as a test. I'll attach it once it works correctly, perhaps I can reuse what you did instead. > BTW with bug 395281 there will be an incompatible change (explicit p2 repo > configuration independent of target platform required for this plugin), see > https://git.eclipse.org/r/#/c/9625/ > > we will announce the change with instructions how to migrate in the release > notes > http://wiki.eclipse.org/Tycho/Release_Notes/0.17 That sounds good thank you. with IT added http://git.eclipse.org/c/tycho/org.eclipse.tycho.extras.git/commit/?id=c0dd700ceefb50c1eb2adb5eefe6462e48939920 I consider this fixed. |