Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 265550

Summary: Review Ant task consistency
Product: [Eclipse Project] Equinox Reporter: Pascal Rapicault <pascal>
Component: p2Assignee: Matthew Piggott <matthew>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: aniefer, mark.melvin
Version: 3.5   
Target Milestone: 3.5 M7   
Hardware: PC   
OS: Mac OS X - Carbon (unsup.)   
Whiteboard:
Attachments:
Description Flags
CSV of attributes used in ant tasks
none
Updated p2 Ant Tasks
none
Ported MetadataRepositories tests for new MirrorApplication
none
Updated p2 Ant Tasks
none
Repository Ant Tasks & Tests
none
Patch with some of the modifications I have done
none
Updated Tasks
none
Tests
none
Updated Tasks
none
Updated Tasks
none
Ant Tasks
none
Tests
none
Ant Tasks
none
Tests
none
testData/mirror/zippedRepo.zip
none
Ant Tasks pascal: iplog+

Description Pascal Rapicault CLA 2009-02-19 16:12:55 EST
Lately we have been writing several ant tasks and it would be good to ensure that there is a degree of consistency among all of these.
One thing that came to mind is, how to identify IU, source repo, target repo, etc.
Comment 1 Pascal Rapicault CLA 2009-02-22 08:34:46 EST
We should also take a look at the input format for repos (URI / URL / path bug 264110).
Comment 2 Matthew Piggott CLA 2009-02-25 11:00:42 EST
I've taken a look at tasks related to repositories (artifact.MirrorApplicationTask , metadata.MirrorApplicationTask, AbstractRepositoryTask, MirrorTask, Rep2Runnable) and the names of attributes are consistent.  

There are some differences in how the tasks handle addresses:
- MirrorApplicationTask converts both destination and source addresses to URLs,  Strings, and finally URIs.  
- AbstractRepositoryTask converts the source address directly to a URI
- AbstractRepositoryTask converts the destination address to a Path, a File, then a URI
- artifact.MirrorApplicationTask converts the baseline address to a URL
Comment 3 Matthew Piggott CLA 2009-02-25 16:15:09 EST
Created attachment 126782 [details]
CSV of attributes used in ant tasks

I believe I've looked through all the p2 ant tasks, and I've compiled a spreadsheet (saved as CVS).  I've found a few items which we may want to look at:

- AbstractPublishTask has an error message stating  Metadata/Artifact Repository must be a URL which is used when it fails to convert a String into a URI
- MirrorApplicationTask (artifact & metadata) uses "writeMode" clean/append, but AbstractPublisherTask and Generator both use "append" true/false
- The CreateComposite..., Validate, Remove/AddChild tasks all use "location" which may be used source, destination, or both.
- The addresses of destination & source repositories are converted several times before they are converted into a URI.  Given the conversions are not always same, could an address be allowable as one input, but not another?

The variations on address conversion are:
String -> URL -> String -> (application converts) URI
String -> Path -> File -> URI -> String
String -> URI
String -> (application converts) URI
Comment 4 Matthew Piggott CLA 2009-03-04 11:46:53 EST
After looking at the mirror tasks, it seems that most of the functionality of artifact.MirrorApplicationTask is also present in tools.MirrorTask.  As both use the same Mirroring class when the mirror operation is performed, I believe the artifact.MirrorApplicationTask could be removed as the version present in tools allows composite repositories, and multiple sources.  The task in tools would need a few attributes added but all should be easy to implement (comparatorID, baseline, ignoreErrors, raw, verbose and writeMode.)

If metadata.MirrorApplicationTask were to extend tools.AbstractRepositoryTask the format for input between artifact and metadata mirroring tasks would be the same.  This change would also require metadata MirrorApplication extend tools.AbstractApplication.  One potential issue is that some input not used by the task (such as an artifact repository) would be valid input but would be completely ignored through the operation.  If this is considered an issue perhaps the unused methods could be overridden and throw an exception or otherwise fail.

The Add/RemoveChildTasks should probably also be modified so that their input would be similar to that used by the mirror tasks.  These tasks could also extend AbstractRepositoryTask, though some thought would be required first to decide if the terms source and destination are appropriate.  If they are not appropriate, a different super class could be created with more appropriate terms.   Either way the change would mean that running a task would be able to add/remove multiple children instead of the multiple tasks required now.  The ValidateTask could also be modified so it could validate multiple repositories with a  single run.

Misc.
- Perhaps AbstractApplication ought to implement IApplication
- I believe SlicingOption is mistakenly extending Task, currently it does not implement execute() and is simply used by the MirrorTask.
Comment 5 Matthew Piggott CLA 2009-03-06 19:48:52 EST
Created attachment 127893 [details]
Updated p2 Ant Tasks

Here's the completed changes to the Ant tasks.  There are a few examples available on the wiki ( http://wiki.eclipse.org/Equinox/p2_Ant_Tasks#Modifying_Children_of_a_Composite_Repository )

I've also modified the new MirrorApplication so it can completely mirror a repository if an IU has not been specified.

Few internal changes still needed:
- org.eclipse.pde.build.AssembleConfigScriptGenerator.generateMirrorTask() to use 'repository' instead of 'destination' tag
- DestinationRepository should probably be renamed to something more generic as its being used for far more than destinations
Comment 6 Matthew Piggott CLA 2009-03-06 19:52:25 EST
Created attachment 127894 [details]
Ported MetadataRepositories tests for new MirrorApplication

Two tests current fail due to changes with default mirror names.  We may want to either update the tests to the new default naming scheme, or change the application to use the old style default.
Comment 7 Matthew Piggott CLA 2009-03-09 10:18:58 EDT
Created attachment 128031 [details]
Updated p2 Ant Tasks
Comment 8 Matthew Piggott CLA 2009-04-08 15:18:13 EDT
Created attachment 131327 [details]
Repository Ant Tasks & Tests

Updated Ant tasks as well as several tests for them.
Comment 9 Pascal Rapicault CLA 2009-04-09 15:55:06 EDT
Some things to change:
  - Externalization of the strings 
  - Make sure PDE Build adopts the new task
  - Reintroduce options and attributes that have been removed and put a note in the code to deprecate it later
Comment 10 Pascal Rapicault CLA 2009-04-09 15:55:48 EDT
Created attachment 131450 [details]
Patch with some of the modifications I have done
Comment 11 Matthew Piggott CLA 2009-04-14 13:13:42 EDT
Created attachment 131810 [details]
Updated Tasks
Comment 12 Matthew Piggott CLA 2009-04-14 13:14:31 EDT
Created attachment 131811 [details]
Tests

Contains tests for tasks, as well as ported versions of Metadata/Artifact Mirror tests for the new MirrorApplication.
Comment 13 Matthew Piggott CLA 2009-04-17 11:21:32 EDT
Created attachment 132255 [details]
Updated Tasks
Comment 14 Matthew Piggott CLA 2009-04-17 16:43:55 EDT
Created attachment 132298 [details]
Updated Tasks
Comment 15 Matthew Piggott CLA 2009-04-20 10:17:22 EDT
Created attachment 132421 [details]
Ant Tasks
Comment 16 Matthew Piggott CLA 2009-04-20 10:18:08 EDT
Created attachment 132422 [details]
Tests
Comment 17 Matthew Piggott CLA 2009-04-22 10:52:30 EDT
Created attachment 132782 [details]
Ant Tasks
Comment 18 Matthew Piggott CLA 2009-04-22 10:53:22 EDT
Created attachment 132783 [details]
Tests
Comment 19 Matthew Piggott CLA 2009-04-22 10:55:11 EDT
Created attachment 132785 [details]
testData/mirror/zippedRepo.zip

Zip repo used in tests: testData/mirror/zippedRepo.zip
Comment 20 Matthew Piggott CLA 2009-04-23 09:46:43 EDT
Created attachment 132929 [details]
Ant Tasks

Updated to add fix for bug 265851.
Comment 21 Pascal Rapicault CLA 2009-04-23 14:34:10 EDT
I have released the last contribution.