Bugzilla – Full Text Bug Listing
| Summary: | Support for other repository | ||
|---|---|---|---|
| Product: | [Eclipse Project] PDE | Reporter: | Pascal Rapicault <pascal> |
| Component: | Build | Assignee: | Pascal Rapicault <pascal> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | benjamin.booth, bugzilla, carsten.pfeiffer, David.Olsen, fg, gunnar, kcoleman, Matthew_Hatem, rainer, ryanman, tlroche |
| Version: | 2.1 | Keywords: | helpwanted |
| Target Milestone: | 3.2 M6 | ||
| Hardware: | PC | ||
| OS: | Windows 2000 | ||
| Whiteboard: | |||
| Attachments: | |||
|
Description
Pascal Rapicault
Changing bug assignment to pde-build-inbox. (pde-core-inbox is obsolete) Hello, I started implementing a patch to make the fetch file generation pluggable. so far I've extracted the CVS specific part into it's own implementation and added a simple fetch script builder for fetching files from a specific location. The location is splitted into ROOT and PATH to allow a custom overwrite of the root via the "fetchTag" property. The only difference is the "cvsPassFile" argument which cannot be supported anymore in the fetch task. But I don't think this is a problem because it wasn't used inside the basebuilder and the eclipsebuilder. It can now be specified inside the MAP files. Not sure if this is a suitable solution or if this should be changed to an overwritable mechanism (like "fetchTag"). I'm also thinking of writing an URL fetch builder that will fetch a zip from a specified URL and extract it into the destination. This might be usefull for tools building against a base Eclipse. Currently they have to write this into the customTargets.xml The current "pluggable" mechanism is not yet implemented by extension points which is still a TODO item. The map file format will not change significantly. Instead the is just an additional entry which must be in front of all other repository specific arguments. It's the repository identifyer. Example feature@org.eclipse.platform=CVS,.... feature@my.custom.feature=PATH,/specificView/vobs/,myVob/features/my.custom.fea ture-feature The simple PATH repository allows a basic support for our ClearCase environment. I just want to let you know, that I'm working on the base support for this. Cu, Gunnar Gunnar, I am really glad to hear that you are starting tackling this problem. I'm not sure I understand what you are doing regarding the fetchTag. But fetchTag should only be used to describe the tag of the things you are getting from the repo (like for example v20040621), although the value is then passed to the script 'as is'. Will the change you are proposing for the map file will allow to keep the eclipse map files unchanged? Why wouldn't we introduce a tag directly in the beginning of the line representing the type of the repository. This way we could say: if there is a tag then use the repo indicated (this would be matched against the available extensions), if not use CVS (this way people using CVS and that already have map files don't have to change anything). This solution would allow to define the map file format to be totally different from a repo to another if necessary. What do you think? Regarding fetching the files from a URL, there is already such a support in the FetchFileGenerator (and FetchFileGeneratorTask) that relies on the map file format from the packager (see bug #32242). Again, I looking forward to see your contribution ! Yes, it's possible to keep the Eclipse map files unchanged (backward compatibility). How can I use the packager? I couldn't find it in the eclipsebuilder or gefbuilder. Created attachment 12626 [details]
Patch to add support for pluggable fetch script bulder
This is a first version of the patch providing the extension point
"org.eclipse.pde.build.fetchScriptBuilder" and two inital builds for fetching
from CVS an from a local PATH (works also with local mapped network shares)
TODO: finalize API in IFetchScriptBuilder and move it into a public package to allow other repository providers start contributing. Backward compatibility is retained. Current Eclipse map files should still work. In general, the new format is: type@element=<BUILDERID>,otherArgs,.. Example: feature@my.feature=PATH,D:/snapshot_views/stable,clearcase_vob/features/my.feat ure-feature Will fetch feature "my.feature" from "D:/snapshot_views/stable/clearcase_vob/features/my.feature-feature". Property "fetchTag" is also supported to define another location. If "fetchTag" is set to "D:\snapshot_views\latest" the effective path would be "D:/snapshot_views/latest/clearcase_vob/features/my.feature-feature". Also supported: plugin@org.eclipse.draw2d=I20040619,:pserver:anon... and plugin@org.eclipse.draw2d=CVS,I20040619,:pserver:anon... ---------- Example extension: ---------- <extension point="org.eclipse.pde.build.fetchScriptBuilder"> <builder class="org.eclipse.pde.internal.build.PATHFetchScriptBuilder" id="PATH"/> <builder class="org.eclipse.pde.internal.build.CVSFetchScriptBuilder" id="CVS"/> </extension> ---------- Thanks for the patch. We will investigate this for inclusion in 3.1. *** Bug 89284 has been marked as a duplicate of this bug. *** Unable to release patch due to backwards compatibility issues. Deferring bug to post-3.1. David Olsen wrote: 1. There are too many variations on fetching source from ClearCase: UCM vs. base-CC, copy from view vs. build directly in view, snapshot view vs. dynamic view, reuse an existing view vs. create a new view with each build. Incorporating that into PDE build would mean either supporting just a couple options, which wouldn't work for all users, or supporting all variations, which would be difficult to control. It also seems like a core Eclipse plugin shouldn't be burdened with all the intricacies of ClearCase. 2. ClearCase just isn't compatible with the existing map files structure. In ClearCase it isn't possible to specify different labels or tags for individual plugins. The unit of labelling is the VOB, which would be at least an entire Eclipse feature, probably a set of related features. So if PDE build were changed to support ClearCase, I think the map files would have to be thrown out all together in favor of some other way of specifying how to fetch the source. So now I am thinking the best approach is to change PDE build so it is easy to disable the entire fetch step, such as by putting noFetch=true in build.properties. Users who disable the fetch would be expected to set up the source directory correctly before invoking PDE build. I'm fine with disabling the fetch step. This is necessary for other people anyway. However the problem is that by doing it you are opting out of the automatic generation of version numbers (unless of course your fetch task creates the expected file for pde to later reconsume). Moreover if the product you are trying to build is spread across various repo (a CVS, CC, etc.), then the fetch task that takes care of following the dependency will not be useful. I was thinking that PDE FetchScriptGenerator task could provide an extension point, thus allowing various repositories to be plugged in and still leveraging the existing code base. As for the map file, it is fine if you have a specific format for CC as long as you can find if it contains a given plugin or feature. (In reply to comment #9) > Unable to release patch due to backwards compatibility issues. Mhm. The patch is backwards compatible. You don't have to change your map files. I'm working with a generic "copy" fetch extension using this patch and ClearCase. Thus, we don't care about labeling etc. All ClearCase related stuff is done before (setting configSpec, updating view) and after (labeling) invoking PDE build. The map files are only used to copy the features and plugins from the ClearCase view to the build directory. We havn't switched to M6 yet but I'm planning to do this soon. Thus, I need to update the patch anyway. Created attachment 22206 [details] [patch] pluggable fetch script builder New version updated to latest changes in org.eclipse.pde.build. This patch adds an extension point for registering fetch script builds than can be used during fetch script generation. It comes with two fetch script builders included: a CVS fetcher and a COPY fetcher. The current map files used within Eclipse.org continue to work. They do not need to be updated. They will use the CVS fetcher by default. To use another fetcher you need to prefix the map file entries with the fetcher id. Example: feature@my.feature=COPY,d:/myrepository,features/my.feature-feature Will generate a script that copies the feature from d:/myrepository/features/my.feature-feature. The 'fetchTag' property can be used to replace 'd:/myrepository' with another path (for example, on another build machine or to fetch from another repository with a different version). (In reply to comment #13) > To use another fetcher you need to prefix the map file entries with the fetcher > id. If the fetcher id is unknown (not found) it will default to the CVS fetcher. Created attachment 30757 [details]
34757_pluggable_fetch_script_builder.patch.txt
New version of the patch against current HEAD
- changed from interface IFetchScriptBuilder to abstract class FetchScriptBuilder
(In reply to comment #15) > Created an attachment (id=30757) [edit] > 34757_pluggable_fetch_script_builder.patch.txt Todo: This patch adds new API to PDE Build and requires some classes/interfaces that are currently internal. These classes needs to be refactored into public packages. - org.eclipse.pde.internal.build.ant.ITask - org.eclipse.pde.internal.build.ant.AntScript Should this be tracked in a separate bug entry? Will consider for 3.2. All, Gunnar, thank you for these changes! Seeing as how they're being considered for 3.2... I've implemented a Perforce P4FetchScriptGenerator that plugs into Gunnar's changes. With help from Luis de la Rosa, our automated builds now run hourly with it. We believe the webMethods' legal hurdle to contributing this back will be minimal. In the meantime, I'm cleaning my changes and javadoc'ing stuff. In addition, the P4FetchScriptGenerator depends on different semantics when using Perforce-style map files. This will need some documentation (or updates to documentation) which I'll be happy to provide. Not being an Eclipse committer, I suppose I'll need to send a patch file for someone to review. Not sure what next... Cheers, Ben The patch looks promising. A few comments: - AntScript and ITask are not API. Do we really want to make those two classes API, can't we restrict what is exposed? - FetchScriptBuilder is not an ideal name. - Needs to specify the wellknown keys that will be part of the map that is being passed around. - How do I specify the final location where I want to thing to be downloaded / put - Code style should use http://www.eclipse.org/eclipse/platform-core/documents/coding_conventions.html (In reply to comment #19) > The patch looks promising. Thanks for your review. > A few comments: > - AntScript and ITask are not API. Do we really want to make those two classes > API, can't we restrict what is exposed? That's one of the open issus. I'll try to create a proposal (patch) for this. > - FetchScriptBuilder is not an ideal name. I'm open for any suggestions. > - Needs to specify the wellknown keys that will be part of the map that is > being passed around. Do you think of documenting the keys? The map isn't currently documented in the main Eclipse documentation. > - How do I specify the final location where I want to thing to be downloaded / > put AFAIK the property ${destination} is set by PDE Build before the fetch script is called. I agree that this should be better documented. > - Code style should use > http://www.eclipse.org/eclipse/platform-core/documents/coding_conventions.html Ok. I will try to rework the patch next week. (In reply to comment #19) > - FetchScriptBuilder is not an ideal name. Any suggestions for a better naming? FetchScriptGenerator is already used by the main generator class. After thinking a while about it, the real task of the FetchScriptBuilder is to create Ant Tasks for the fetch scripts. Thus, it's not really a fetch script builder but a fetch task factory. Possible candidates are: FetchTaskFactory FetchScriptTaskFactory I'm ok with both names but would use the larger one for a better clarity and conformity with existing classes. This will also change the name of the extension point. Any opinions/objections/other proposals? (In reply to comment #21) > (In reply to comment #19) > > - FetchScriptBuilder is not an ideal name. > > Any suggestions for a better naming? FetchScriptGenerator is already used by > the main generator class. > > After thinking a while about it, the real task of the FetchScriptBuilder is to > create Ant Tasks for the fetch scripts. Thus, it's not really a fetch script > builder but a fetch task factory. > > Possible candidates are: > FetchTaskFactory > FetchScriptTaskFactory > > I'm ok with both names but would use the larger one for a better clarity and > conformity with existing classes. This will also change the name of the > extension point. > > Any opinions/objections/other proposals? > Gunnar, You could also do FetchTaskCreator. For whatever this is worth, I like FetchTaskFactory though; "factory" is a more standard term. FetchScriptTaskFactory is unecessarily verbose. -ben Gunnar, sorry for the late reply.
I'm fine with either FetchTaskFactory or FetchTaskCreator. It is your choice.
>keys in the map
When I'm talking about the keys in the map. I'm not referring to the actual format of the map files, but to the object that is being passed to the factories and back to the generator.
Do you think you can have the patch ready by the end of the week?
Benjamin, does your perforce factory use the standard perforce ANT tasks? Are you still ok to provide us with a patch once Gunnar will be done with his (this can wait after M5). (In reply to comment #24) > Benjamin, does your perforce factory use the standard perforce ANT tasks? > Are you still ok to provide us with a patch once Gunnar will be done with his > (this can wait after M5). > Yes, it uses standard perforce Ant tasks. My plugin to Gunnar's code also uses a Perforce-specific modification I made to AntScript. Imo, AntScript should be public API with minor extensibility improvements to keep from putting every SCM's Ant creation code in AntScript.java. Here are the requirements to use the Perforce Ant commands: http://ant.apache.org/manual/OptionalTasks/perforce.html Of importance, it requires the jakarta-oro JAR. Yes, I'm very interested in providing a patch. Just say where and when. Created attachment 33845 [details]
34757_pluggable_fetch_script_builder.patch.txt
Here is the new patch.
- Renamed FetchScriptBuilder to FetchTaskFactory
- Renamed extenion point to "org.eclipse.pde.build.fetchTaskFactories"
FetchTaskFactory
- Modified FetchTaskFactory to return CharSequence objects (eg. String, StringBuffer), i.e. no need to make AntScript and ITask API
- Added constansts + JavaDoc for well known (common) map keys to
- Reformated to Platform Core style conventions (hopefully)
Pascal, please review.
Created attachment 33847 [details]
34757_pluggable_fetch_script_builder.patch.txt
reformated FetchTaskFactoriesRegistry
Comments on the last patch. Once those will be fixed I will likely integrate. FetchTaskFactoriesRegistry - Need to use the PDE Build constant - The synchronization is not correct. For example in getFactory(String id) the initializeRegitry() code can return, another thread can invoke the registry changed event, which will then cause the line IConfigurationElement extension = (IConfigurationElement) factories.get(id) to NPE. FetchScriptGenerator: - updates the comment to talk in terms of factory and not builder Does CVS factory fails gracefully if given something bogus to parse (I haven't dug in the code)? AntScript / ITask: The code still uses ITask to then transform it in a CharSequence? Maybe we should just make AntScript and ITask API, pass the script to the factory and tell people to use AntScript#print(ITask) I have not reviewed in details the other changes to AntScript, FetchScriptGenerator as the diff was impossible because the methods have been sorted. In the next patch you provide make sure you don't sort the methods. Thx. Created attachment 34178 [details] 34757_pluggable_fetch_script_builder.patch.txt (In reply to comment #28) > Comments on the last patch. Once those will be fixed I will likely integrate. Great. :) > FetchTaskFactoriesRegistry > - Need to use the PDE Build constant Done. > - The synchronization is not correct. For example in getFactory(String id) the > initializeRegitry() code can return, another thread can invoke the registry > changed event, which will then cause the line IConfigurationElement extension = > (IConfigurationElement) factories.get(id) to NPE. I see the problem. Do you think we need synchronization at all? I thought a bit about this. Usually PDE Build is invoked from the command line only for building. Usually, this is a single process with no registry updates through installing and removing bundles. It looks like we don't even need to support the dynamic registry. What do you think? Should I use a synchronized HashMap? IMHO performance is not critical at this stage of PDE Build. > FetchScriptGenerator: > - updates the comment to talk in terms of factory and not builder Done. > Does CVS factory fails gracefully if given something bogus to parse (I haven't > dug in the code)? It's the same code that was used before in FetchScriptGenerator. See below. > AntScript / ITask: > The code still uses ITask to then transform it in a CharSequence? Yes, but that's only internal code. I did this to avoid a rewrite of the CVS fetch code and to not make AntScript/ITask API. > Maybe we should just make AntScript and ITask API, pass the script to the > factory and tell people to use AntScript#print(ITask) Do we really want to make this API? In this case we should make the whole package API. I think a lot people will love us because it's a great API to write build scripts. The package API is mature and proven to be stable and useful. I don't know your process in detail. Should I open a new bug report to let the PMC decide about this? If yes I'm ready to contribute a patch for this and update this patch. > FetchScriptGenerator as the diff was impossible because the methods have been > sorted. In the next patch you provide make sure you don't sort the methods. Fixed. I accidentally hit the sort action. Created attachment 34179 [details]
34757_pluggable_fetch_script_builder.patch.txt
updated to HEAD and fixed JavaDoc
The problem I see with making AntScript API is that it will limit our ability to change it (historically I've evolved the class doing wild renaming or method signature change), it does not contain all the ant tasks (and will never) and finally for a given task it does not have all the parameters.
On the other side not making AntScript API is annoying as I don't find the manipulation of CharSequence to be very elegant.
Maybe should we:
- create a new API interface named IAntScript that has a printTask(ITask) method
- make AntScript implements this interface.
> I think a lot people will love us because it's a great API to write build scripts.
As mentionned earlier we are not complete and we can not really be as Ant is open-ended and evolve. Moreover I don't think there is that many people generating AntScripts, and would they be interested in requiring PDE Build (and all its prereqs?). I think a better owner for that might be Platform / Ant. I discussed it with the eclipse ant lead a year ago but we never went forward with it.
I'm fine with not tracking the extension registry while we are running. However it would be good to clean the cache at the end of the run of the fetchScriptGenerator and initialize on startup.
It might be usefull to give the FetchTaskFactory a place where it can write arbitrary content to the ant script. In particular, to write a target that might be used by the fetch, retrieveFile & authentication tasks. public CharSequence getCustomTargets(); FetchScriptGenerator would write this to the script before the generateEpilogue() call. Also, the AntScript file is still sorted. Created attachment 34327 [details] 34757_pluggable_fetch_script_builder.patch.txt (In reply to comment #31) > Maybe should we: > - create a new API interface named IAntScript that has a printTask(ITask) > method > - make AntScript implements this interface. Done. There is now an interface IAntScript with basic print methods. This interface is passed to an AntTask (former ITask interface). The AntTask uses the IAntScript for printing content to the Ant script. I change the interface ITask to an abstract class AntTask. This allows us to introduce additional methods later while maintaining backwards compatibility. IAntScript and AntTask are now API (updated MANIFEST.MF). > I think a better owner for that might be Platform / Ant. I > discussed it with the eclipse ant lead a year ago but we never went forward > with it. I agree. Ant tooling is the better owner. We might have a full AST one day that we can use for creating build files. :) > I'm fine with not tracking the extension registry while we are running. However > it would be good to clean the cache at the end of the run of the > fetchScriptGenerator and initialize on startup. Done. The registry is no longer tracked. I also added a call in FetchScriptGenerator #generate to close/clean up the factories registry. (In reply to comment #32) > It might be usefull to give the FetchTaskFactory a place where it can write > arbitrary content to the ant script. In particular, to write a target that > might be used by the fetch, retrieveFile & authentication tasks. > public CharSequence getCustomTargets(); > FetchScriptGenerator would write this to the script before the > generateEpilogue() call. Done. I also updated the JavaDoc in the methods of FetchTaskFactory to indicate wheter implementors are expected to create Ant targets themself or if it's prohibited. > Also, the AntScript file is still sorted. Ah, thanks, I always wondered why the patch was still that large. Fixed now. @all Sorry for any inconvenience while changing the API in this patch so often. If there are any adopters, please test this now. I feel pretty comfortable with the API right now. We use the integrated COPY fetcher in our ClearCase environment. Moving to Pascal for review comments. :) Today is the last day before the API freeze. Any chance that we can get this patch in? Gunnar Wagenknecht 2006-02-10 09:43
> Today is the last day before the API freeze. Any chance that we can
> get this patch in?
PLEASE DO! even if it's not _absolutely_ perfect, suitable for
engraving on stone tablets for eternity ...
Harrassing me by email or bug report does not help. Instead it allienates me. Currently I'm working with Oleg on some other issues related to the registry that have a bigger impact since it is a much more visible API. This bug will be fixed on time, I know my priorities. Thanks to Gunnar's help, this bug is now fixed. I just released the code in HEAD (it is also tagged as post_34757 and the code without the change is tagged before_34757). The final version of the code is slightly different from the last patch to address some limitations and simplify / cleanup the APIs. Any feedback on the new API is greatly appreciated. I did not release the copy extension because I did not convert it to the new APIs. Gunnar, it would be great if you could update it so I can integrate it post M5? (In reply to comment #38) > Any feedback on the new API is greatly appreciated. I like the API. I have two questions on the implementation. Some tasks moved from AntScript to CVSFetchTaskFactory and one was duplicated. I agree that it's much more cleaner to keep the CVS specific tasks out of AntScript. But isn't it easier to keep the printAvailable task in AntScript and simply cast do the internal AntScript? Or did you duplicate it to have a cleaner example code? I wonder how to handle this in the copy extension (duplicate or cast to AntScript). The second question is related to the overriding of the fetch tag. The OVERRIDE_TAG is specified in lower case. But the comment in build.properties specifies it uppercase. Isn't java.util.Properties case sensitive? Another thing that should be document or changed is in FetchScriptGenerator#setFetchTagAsString. If the fetchTag is set to "SVN=trunk" it's interpreted as an overwrite for CVS (because entries.length == 1 evaluates to true). Thus, one has to specify at least two entries (eg. "SVN=trunk, CVS=HEAD") > I did not release the copy extension because I did not convert it to the new > APIs. Gunnar, it would be great if you could update it so I can integrate it > post M5? I converted it but will verify it tomorrow in our build before attaching it. In the antscript I removed the cvs tasks because they are only used by the CVS factory. As for the available task I copied it in order to have a clean example. I think you should do the same in the copy task. You are right about the value for the override. I tested it and then renamed :-). This has been fixed in HEAD. Created attachment 34649 [details]
34757_copy_extension.patch.txt
Here is the copy extension adapted to the new API.
Created attachment 34650 [details]
34757_fetchTag_fix.patch.txt
This patch fixed an issue with fetchTag backwards compatibility.
Created attachment 34651 [details]
34757_copy_extension.patch.txt
Just a small change to beatify the generated fetch script. :)
I don't think the copy factory should worry about checking if the various descriptor files are here since the copy task will only copy files if the source is newer than the destination. In the CVS case this check is only done because we don't do a checkout, but an export and we therefore don't have the cvs info to skip uncessary downloads. If you remove the check of the copy factory, then I suggest that you remove FetchFromDisc target and directly inline the calls to the copy task. This would have the added benefit of making the code more straightforward and also showing another example very different from the CVS factory. Thanks for the fetchTag patch and the catching other typos. They are already fixed in HEAD. Created attachment 34743 [details]
34757_copy_extension.patch.txt
I agree. It's good to have a different example.
I've just released the patch for the copy task in HEAD. It will be part of M6. I don't think we should make it "API" for people using it in their map files as there are still some issues that are left unanswered (for example what is the "tag"). I have opened the bug #128487. [contributed patch applied] |