Bugzilla – Full Text Bug Listing
| Summary: | Target Export wizard does not produce repos | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] PDE | Reporter: | Jeff McAffer <jeffmcaffer> | ||||||||||
| Component: | UI | Assignee: | Curtis Windatt <curtis.windatt.public> | ||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | bernhard.merkle, curtis.windatt.public, irbull | ||||||||||
| Version: | 3.6 | Keywords: | contributed, noteworthy | ||||||||||
| Target Milestone: | 3.7 M4 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Mac OS X - Carbon (unsup.) | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Jeff McAffer
Created attachment 182251 [details] patch to export repos and correct content This patch replaces the current export active target function with an ExportTargetJob that - exports any target - exports p2 metadata and artifact repos for those target elements coming from a Software Site - exports only the things that are actually enabled in the target (selected on the Content page) The patch overlaps and depends somewhat on the consolidated patch from bug 328929. I believe that the profile manipulation etc actually requires some of the fixes in the other patch. Since Curtis is looking at that consolidated patch right now, I figure I'll wait and update when/if he commits the consolidated stuff and then resolve conflicts for this patch and issue a new one. I'm attaching this now so people interested can get a look at the code and potentially give feedback. Topics: - I'm a little unsure of when you can "resolve" a target and the ramifications of doing so. The patch resolves targets before they are exported. I seem to be able to resolve things without affecting the current target but there may be something I'm missing. - Progress monitoring is less than optimal in this patch. - There may be a few strings to be NL'd. Created attachment 182513 [details]
updated patch + some profile caching improvements
This new patch has the export function on top of the changes Curtis recently released (consolidated patch).
There are also a number of tweaks to the way we cache profiles and targets. While it looks like a lot of change, really it is the following
- a mess of code moved from IUBundleContainer to P2TargetUtils
- the per container storage of allrequired, allenviroinments and include source have been eliminated in favour of per-target values
- persistence has been *partially* updated. I made the 36 persistence helper store the new form but likely we need a new 37 persistence helper.
Note that progress monitoring is hosed. I'm not that great at managing progress monitors, etc. I'm happy to be guided or to let someone else polish there.
A few questions/concerns about the patch: Export: Export code changes look reasonable, but I'm having an odd problem while trying to test them in a target. The wizard class can't be instantiated. Looking at the code, my only suggestion is to get rid of the export button on the editor. The button looks completely out of place since it has nothing to do with the selection and we already have access to the wizard through the standard export framework. If an export button is really necessary, we should have an icon on the top right for it, the same way that we do in the plug-in and feature editors. The rest: 1) All the other changes are making it harder to review this patch. The patch overrides my changes to the messages, so I don't know what other changes it may have overwritten. I would prefer to move these changes to a new bug where we can discuss the implications. 2) I am very uncertain about the changes to the ITargetDefinition API. Why do we need a sequence number? How come source inclusion is a global setting when it can differ per container. I am not seeing the value in the changes. Could we leave the settings on the containers, but just have some custom code to resolve all the bundle containers at once? 3) As you already pointed out, for any of these file structure changes, we'll need a new version of the file and a new 37persistancehelper. (In reply to comment #3) > Export code changes look reasonable, but I'm having an odd problem while trying > to test them in a target. The wizard class can't be instantiated. Looking at > the code, my only suggestion is to get rid of the export button on the editor. > The button looks completely out of place since it has nothing to do with the > selection and we already have access to the wizard through the standard export > framework. If an export button is really necessary, we should have an icon on > the top right for it, the same way that we do in the plug-in and feature > editors. Hmm, not sure what is going on about the instantation. I believe that I was doing this while testing it before. The point of having the button in the editor is to support what at least I see as a common workflow. Tweak the target and then export it. Basically the same as "Set as current target platform". In any event, if it works better to have it as a button at the top, that's fine for me. > 1) All the other changes are making it harder to review this patch. The patch > overrides my changes to the messages, so I don't know what other changes it may > have overwritten. I would prefer to move these changes to a new bug where we > can discuss the implications. The export related changes are pretty discrete. Are you wanting then a separate patch just for those or can you see what is needed there and commit just those? If that works then I'll make a new patch for the other changes. Whatever works for you. > 2) I am very uncertain about the changes to the ITargetDefinition API. Why do > we need a sequence number? The sequence number allows us to corelate a TargetDefinition to a profile. The sequence number changes each time the target undergoes a change that affects the set of features or bundles that the target defines. The profile remembers the sequence number of the target it represents. If they match then all is well. If they don't, the profile is thrown away. This makes the target def the locus of control and the profile somewhat disposable. > How come source inclusion is a global setting when it can differ per container. Actually, it can't. It used to be per container but like the includeRequired and includeAllEnvironments settings, it affects the way that bundles are resolved (ie., the way the profile is manipulated). Therefore, it is global to the target def. > I am not seeing the value in the changes. Could > we leave the settings on the containers, but just have some custom code to > resolve all the bundle containers at once? Potentially but my thinking was that that was more confusing/complicated. There was all sorts of code that would scan the containers to make sure they all had the same settings etc. This approach simplified this and represents the actual semantics that are implemented. > 3) As you already pointed out, for any of these file structure changes, we'll > need a new version of the file and a new 37persistancehelper. So what's the process for doing that? i don't know enough of the context or expectations to do that reliably. The patched 36 helper does the right persistence. do we just move all that to a 37 helper or do changes need to be done to the other helpers or ... Created attachment 183620 [details]
Target Export Patch
Moves export button to the top of the editor. Needs a bit more testing as I am still having problems instantiating the wizard from the platform export dialog.
Also: 1) The wizard should remember it's previous destination in dialog settings. 2) If a non-p2 target (directory) is exported, an error is thrown suggesting that the export failed, when in fact it completed. Two quick things Jeff: 1) When adding required bundles to the manifest you need to set a version range (this does not happen by default yet). 2) For wizards that are instantiated by the platform (through extensions) you must have a default constructor. That's why I was seeing instantiation problems with the patched code. Created attachment 183695 [details]
Final Target Export Fix
Applied the last patch to HEAD. It stores previous settings in dialog settings (up to three previous destinations) and includes a few other improvements. This was a significant improvement over the previous export. Marking as VERIFIED. I didn't add a N&N entry for it, since it is not a new feature for the user. Hmmm, I added a n&n entry in the draft you sent. There are a couple things that were added here. First, repos were being exported. they were not before. Second, you can export any target, not just the active one. Anyway, feel free to delete the N&N entry. *** Bug 328924 has been marked as a duplicate of this bug. *** |