Bugzilla – Full Text Bug Listing
| Summary: | [prov] ECFTransport and ECFMetadataTransport should be merged | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Tim Mok <timothym> | ||||||||||
| Component: | p2 | Assignee: | P2 Inbox <equinox.p2-inbox> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | dj.houghton, henrich.kraemer, henrik.lindberg, john.arthorne, pascal, remy.suen | ||||||||||
| Version: | 3.4 | ||||||||||||
| Target Milestone: | 3.5 M7 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Windows XP | ||||||||||||
| Whiteboard: | |||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 248604, 266243, 269503, 269588, 270496 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Tim Mok
Where would such a merged class live? If it is moved out of the repo bundles, it will likely push the dependency on ECF deeper into p2. If the overlap is small, it might not be worthwhile. There doesn't seem to be a good place to put this merged class. I don't think it can live in ECF since the artifact repo needs some special checks related to artifact streams. Maybe creating some kind of communication bundle or package where an abstract Transport class can reside with ECFTransport subclasses. Then the dependency on ECF can be abstracted from p2 along with allowing other methods of retrieving files. Pascal mentioned that it's worth merging the classes since the code is virtually the same. I'll also need to add code for timestamp checks, which artifact repositories can get for free. Ideally this code would also be used by the updatesite repository bundle. There's also a org.eclipse.equinox.internal.p2.updatesite.ECFTransport, should all three be merged? Yes, they should all be merged. There isn't much of a coding problem here, as the three transport classes are largely the same implementation. The only problem here is where would the common class live? p2.core is the only common place but pushing the ECF dependency down to core seems limiting. A separate bundle for ~300 lines of code also doesn't make much sense. We are making changes to these classes to address various transport issues. This is a nightmare. We have bundles that are smaller than these (e.g. examplary setup) :). Could it be moved to ECF? Reassigning to Henrik I am working on this. A new bundle o.e.e.p2.repository was suggested, so I am using that. When merging the three implementations I am also improving robustness, handling of exceptions, and making sure that users get the real cause of issues instead of intermediate and meaningless info. There are also issues with monitoring and cancelling in certain cases. Created attachment 128610 [details]
Implementation of the new bundle with common repository code
This is the implementation of a merged transport and improved error handling in the transport layer.
As this is a new bundle it is not possible to provide it as a patch. A patch is to follow that makes use of this new bundle.
Created attachment 128612 [details]
Patch to use new repository bundle
This is a refactoring of the repository packages in core to a new separate bundle.
It also makes use of improved error handling in the new repository bundle.
There is no regression when running automated tests (same testcases fail in the same way as without the patch). Signing over to p2-inbox to get a p2 committer to work on this. Comment "HENRIK: TODO" in In SimpleMetadatRepositoryFactory.getLocalFile // TODO HENRIK: Cause of problem is unknown (can be Unknown Host, etc.) - this must be communicated. should be removed - this has been handled by an earlier method throwing more detailed exceptions. Forgot to remove this comment. I have released the bundle in HEAD. I have looked at applying the patch but I get conflicts on the following files. These likely results from cahnges that have happened since you "forked". For example Simon has been working on some parser improvements here and there. Could you please provide a new patch? thx. p2.core AbtractRepoManager CompositeParser CompositeWriter XMLParser XMLWriter engine Metadata is now gone. Don't bother about it. reconciler.dropins PlatformXmlListner Created attachment 129128 [details]
Patch to use the new repository bundle (updated).
Thanks Pascal. An updated patch is attached.
When I apply the patch, the following test is failing: org.eclipse.equinox.p2.tests.updatesite.AllTests. Could you please see what is going on? Thx. I have been running AutomatedTests, and I get the same 19 errors both before and after applying the patch. The errors were all in reconciliation, and one in full SDK install. You get errors in reconciler tests because you need to specify the path to an SDK zip (see the launch config). However the problem is not there. On the mac, the problems I see are: junit.framework.AssertionFailedError: 0.2: org.eclipse.equinox.internal.provisional.p2.core.ProvisionException: Error reading update site jar:file:/Users/Pascal/dev/explanation/org.eclipse.equinox.p2.tests/testData/updatesite/digest/site.zip!/. at junit.framework.Assert.fail(Assert.java:47) at org.eclipse.equinox.p2.tests.AbstractProvisioningTest.fail(AbstractProvisioningTest.java:548) at org.eclipse.equinox.p2.tests.updatesite.UpdateSiteTest.testZippedDefaultDigestURL(UpdateSiteTest.java:115) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:585) at junit.framework.TestCase.runTest(TestCase.java:164) at org.eclipse.equinox.p2.tests.AbstractProvisioningTest.runTest(AbstractProvisioningTest.java:753) at junit.framework.TestCase.runBare(TestCase.java:130) at junit.framework.TestResult$1.protect(TestResult.java:106) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.framework.TestResult.run(TestResult.java:109) at junit.framework.TestCase.run(TestCase.java:120) at junit.framework.TestSuite.runTest(TestSuite.java:230) at junit.framework.TestSuite.run(TestSuite.java:225) at junit.framework.TestSuite.runTest(TestSuite.java:230) at junit.framework.TestSuite.run(TestSuite.java:225) at junit.framework.TestSuite.runTest(TestSuite.java:230) at junit.framework.TestSuite.run(TestSuite.java:225) at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390) at org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner.main(RemotePluginTestRunner.java:62) at org.eclipse.pde.internal.junit.runtime.CoreTestApplication.run(CoreTestApplication.java:23) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:585) at org.eclipse.equinox.internal.app.EclipseAppContainer.callMethodWithException(EclipseAppContainer.java:574) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:368) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:585) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:556) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:511) at org.eclipse.equinox.launcher.Main.run(Main.java:1284) at org.eclipse.equinox.launcher.Main.main(Main.java:1260) junit.framework.AssertionFailedError: 0.2: org.eclipse.equinox.internal.provisional.p2.core.ProvisionException: Error reading update site jar:file:/Users/Pascal/dev/explanation/org.eclipse.equinox.p2.tests/testData/updatesite/goodfeatureurl/site.zip!/. at junit.framework.Assert.fail(Assert.java:47) at org.eclipse.equinox.p2.tests.AbstractProvisioningTest.fail(AbstractProvisioningTest.java:548) at org.eclipse.equinox.p2.tests.updatesite.UpdateSiteTest.testZippedGoodFeatureURL(UpdateSiteTest.java:357) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:585) at junit.framework.TestCase.runTest(TestCase.java:164) at org.eclipse.equinox.p2.tests.AbstractProvisioningTest.runTest(AbstractProvisioningTest.java:753) at junit.framework.TestCase.runBare(TestCase.java:130) at junit.framework.TestResult$1.protect(TestResult.java:106) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.framework.TestResult.run(TestResult.java:109) at junit.framework.TestCase.run(TestCase.java:120) at junit.framework.TestSuite.runTest(TestSuite.java:230) at junit.framework.TestSuite.run(TestSuite.java:225) at junit.framework.TestSuite.runTest(TestSuite.java:230) at junit.framework.TestSuite.run(TestSuite.java:225) at junit.framework.TestSuite.runTest(TestSuite.java:230) at junit.framework.TestSuite.run(TestSuite.java:225) at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390) at org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner.main(RemotePluginTestRunner.java:62) at org.eclipse.pde.internal.junit.runtime.CoreTestApplication.run(CoreTestApplication.java:23) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:585) at org.eclipse.equinox.internal.app.EclipseAppContainer.callMethodWithException(EclipseAppContainer.java:574) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:368) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:585) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:556) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:511) at org.eclipse.equinox.launcher.Main.run(Main.java:1284) at org.eclipse.equinox.launcher.Main.main(Main.java:1260) (In reply to comment #19) > You get errors in reconciler tests because you need to specify the path to an > SDK zip (see the launch config). However the problem is not there. > > On the mac, the problems I see are: > junit.framework.AssertionFailedError: 0.2: > org.eclipse.equinox.internal.provisional.p2.core.ProvisionException: Error > reading update site > Thanks Pascal, I have been debugging this for a while now, and I think I have narrowed down the problem. Seems like some use of Transport expects it to close the output stream, but not all. By Making a small change I got rid of the two errors that caused the test to fail (file ended prematurely). When I did that however, another tests failed. Almost done, but it means that the difference close/do not close needs to propagate up to where the new RepositoryTransport is used. Not a big deal - it is all internal API, but it will take me a few hours more to fix. (Hopefully it is the close/do not close that is causing the new problem I found). Created attachment 129407 [details]
Updated patch - fixed issue with close/not close outputstream
Updated patch that fixes the issues with failing tests. This was caused by the fact that UpdateSite's ECFTransport always wanted the output stream to be closed on finish, while the other ECFtransports did not. The fix introduces an optional flag to Transport.download().
Thx for the patch Henrik! I have released it with the slight modification that I have not used the download method with the close parameter. Instead I have changed the 3 callers in the updatesite bundle to close the stream. (In reply to comment #22) > Thx for the patch Henrik! I have released it with the slight modification that > I have not used the download method with the close parameter. Instead I have > changed the 3 callers in the updatesite bundle to close the stream. > Great. The change makes it cleaner. |