Community
Participate
Working Groups
When importing a bundle via SCM URL, TeamUI#getPages[1] expects fileSystemScheme [2] to be defined for a repository provider. First the URI is checked whether its scheme equals "scm": if (ProjectSetCapability.SCHEME_SCM.equals(scmUri.getScheme())) { ... Then the rest of the URI is checked if it starts with repository provider's file system scheme: if (scmUri.getSchemeSpecificPart().startsWith(providerScheme)) { ... While the former check is fine, the latter causes repository providers which don't provide the file system extension [2] to be skipped. The file system doesn't take part in the import operation whatsoever, so ignoring providers without it (eg. git [3]) is wrong. [1] org.eclipse.team.ui.TeamUI.getPages(ScmUrlImportDescription[]) [2] a file system registered with the org.eclipse.core.filesystem.filesystems ext point [3] org.eclipse.egit.core.GitProvider, see also https://git.eclipse.org/r/#/c/4899/2/org.eclipse.egit.core/plugin.xml
Created attachment 211278 [details] Fix v01 Changes in the patch: * updated scmUrlImportPages ext point, so page definition has a reference to a bundleImporter not repository * added a new method to Team, ie getBundleImporter(String) which returns a bundle importer for the given ID * added isSupported(URI) method to the IBundleImporterDelegate interface which allows to check if the given URI is supported by the importer. The check is done basing on importer's supported values from its definiton * removed setter and getter in IScmUrlImportWizardPage as they were never used * updated tests * and finally, changed TeamUI#getPages so pages are added basing on the result of isSupported(URI) check
Created attachment 211279 [details] mylyn/context/zip
TODO: add compatibility problem filters for changes from the patch as most of them are breaking API. Even though it's provisional.
Created attachment 211423 [details] Fix v02 Less API changes; use importer kept under "BUNDLE_IMPORTER" key in properties of an import description, added by PDE's BundleProjectService; see also bug 372250
(In reply to comment #4) > Created attachment 211423 [details] [diff] > Fix v02 > > Less API changes; use importer kept under "BUNDLE_IMPORTER" key in properties > of an import description, added by PDE's BundleProjectService; see also bug > 372250 I wanted to review the patch but stopped after seeing API Tools errors. Don't you see them?
Created attachment 211503 [details] Fix v02 + API filters (In reply to comment #5) > I wanted to review the patch but stopped after seeing API Tools errors. Don't > you see them? Sorry, I do see them, that was meant to be a quick patch to share with Szymon. Definitely not for committing yet. The reported errors are due changes made to provisional API, so I'm adding filters to ignore them. Initally, I wanted Szymon to have a look at it but if you could review the patch I would be grateful.
Created attachment 211661 [details] Fix v03 New changes in the fix: * Szymon suggested to hide Team#getBundleImporters() and newly added #getBundleImporter(String) methods by marking them with @noref tag. * The above changed the required API filters. Updated in the patch. The request for moving the key[1] for storing an importer in import description to Team is bug 372250. [1] org.eclipse.pde.internal.core.project.BundleProjectService.BUNDLE_IMPORTER
(In reply to comment #7) > Created attachment 211661 [details] > Fix v03 > > New changes in the fix: > * Szymon suggested to hide Team#getBundleImporters() and newly added > #getBundleImporter(String) methods by marking them with @noref tag. > * The above changed the required API filters. Updated in the patch. > > The request for moving the key[1] for storing an importer in import description > to Team is bug 372250. > > [1] org.eclipse.pde.internal.core.project.BundleProjectService.BUNDLE_IMPORTER The fix looks good.
> * Szymon suggested to hide Team#getBundleImporters() and newly added > #getBundleImporter(String) methods by marking them with @noref tag. -1 for that. There are - though experimental - clients that use this API (including Team itself) and I think the goal is to make it public at one day - no? I would leave core.team as it is now and simply let the client(s) implement that simple loop to find the importer. To array conversion should create an array of the correct length (not 0) The schema doc needs some work: - the attribute has no description - the example is not updated Tomek and I also chatted about some other modification including how to deal with the internal PDE constant. Tomek will attach a new patch in a day or two.
(In reply to comment #9) > > * Szymon suggested to hide Team#getBundleImporters() and newly added > > #getBundleImporter(String) methods by marking them with @noref tag. > > -1 for that. There are - though experimental - clients that use this API > (including Team itself) and I think the goal is to make it public at one day - > no? I would leave core.team as it is now and simply let the client(s) implement > that simple loop to find the importer. I think that marking something experimental means it may or may not become API at one day. So my point is, we do not have any external client for this API, so we should hide it. So I still think these methods should be hidden and the rest (except problems in impl Dani pointed out) is fine. Regarding "some other modification including how to deal with the internal PDE constant". If PDE does not use the constant for other purposes, I don't see why we can't move it to Team.
(In reply to comment #10) > (In reply to comment #9) > > > * Szymon suggested to hide Team#getBundleImporters() and newly added > > > #getBundleImporter(String) methods by marking them with @noref tag. > > > > -1 for that. There are - though experimental - clients that use this API > > (including Team itself) and I think the goal is to make it public at one day - > > no? I would leave core.team as it is now and simply let the client(s) implement > > that simple loop to find the importer. > > I think that marking something experimental means it may or may not become API > at one day. So my point is, we do not have any external client for this API, so > we should hide it. No, we must not. The method is part of the provisional API that your experimental clients have to use, so @noref tag makes no sense. > Regarding "some other modification including how to deal with the internal PDE > constant". If PDE does not use the constant for other purposes, I don't see why > we can't move it to Team. Because the property is there to identify the importer - PDE in this case. PDE sets the property and later queries its importers. If that's not needed i.e. if we only support one kind of importer (i.e. PDE importer), then we can drop the constant and the property altogether.
Tomek, after another chat with Szymon, I think we can even reduce this to TeamUI.getPages(String importerID). PDE Core creates the valid descriptors and can use whatever property (key) it wants to communicate with PDE UI, but Team does not know about this: it simply has to serve the pages for the given importer.
Created attachment 211693 [details] Fix v04
Created attachment 211694 [details] Fix for PDE
(In reply to comment #13) > Created attachment 211693 [details] > Fix v04 Pushed to Gerrit: https://git.eclipse.org/r/#/c/5176/
The patch looks good for Team and PDE. I would have extracted "importer.getId()" into a local variable, but will do that with a commit afterwards. Submitted the Team patch via Gerrit. Fixed in PDE with: http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=7f995f33c29c4fff54b6c6faa0930da7e6d0c17f When testing it, I ran into some additional problems (see bug 372726 for details).
