Bugzilla – Full Text Bug Listing
| Summary: | Use synchronization for non-CDT projects | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] PTP | Reporter: | Greg Watson <g.watson> | ||||||||||
| Component: | RDT.sync | Assignee: | Greg Watson <g.watson> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | beth, c.karbach, com-eclipse-dot-org, jeblen | ||||||||||
| Version: | 7.0 | ||||||||||||
| Target Milestone: | 7.0 | ||||||||||||
| Hardware: | Macintosh | ||||||||||||
| OS: | Mac OS X | ||||||||||||
| Whiteboard: | |||||||||||||
| Bug Depends on: | 408496 | ||||||||||||
| Bug Blocks: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Greg Watson
Created attachment 229522 [details]
sync patch
As a first cut at this, I have separated the CDT/non-CDT specific components into to new plugins. I have also added a new extension point 'synchronizePolicy' which associates a policy with a project nature. The extension provides a class that implements ISynchronizePolicy. I've also reorganized the plugin packages to conform to the Eclipse standards.
Created attachment 229523 [details]
updated patch including new plugins
Created attachment 229525 [details]
updated patch
Created attachment 229531 [details]
updated patch to make it easier to apply
I haven't been able to apply the patches successfully, so I created a new branch called 'new_sync' instead. Please check out this branch to see the changes. John, can you look at this asap? I'd like to get the go/no go before merging into master becomes too difficult. Thanks. After discussion, we agreed on the following changes: 1) Rename BuildScenario to SyncConfig 2) Add extension point to intercept syncConfig create and delete events (passes project and config) 3) Add extension point to intercept syncConfig change (passes project, old config, and new config) 4) Add properties page for sync configs - includes pre and post build options 5) Persist this property data in the project store 6) Add listeners for pre and post build events 7) Drop the sync properties page under C/C++ Build. 8) Add generic sync properties page to create/remove/edit sync configurations. 9) Provide UI elements that project wizards can use. 10) Provide a convenience method to convert a project to synchronized. On reflection, I think we can achieve 2 & 3 without an extension point. 1) Completed 2) Use SyncConfigManager#addSyncConfigListener to register a listener for changes to sync configurations 3) Same as 2 4) New properties page added 5) Completed 6) Not yet completed 7) Completed 8) Completed 9) Not yet completed 10) Use SyncManager#makeSyncProject In addition, I've replaced the SYNC service configuration with a new synchronizeService extension point and converted the Git provider over to use this new extension. I've added a generic new project wizard and this appears to be working. John, please take a look and let me know if I've missed anything. If you would like to start converting the CDT support over to the new interface, please do so. Beth recently committed some changes to master, so I'll merge these with the new_sync branch. I'll also continue to work on missing core functionality. 9) Added SyncProjectWidget that encapsulates the local and remote project location information. I reviewed the changes, and it looks very good overall. I have only a few comments about the new SyncConfig and SyncConfigManager classes:
1) I think there is a bug in "loadConfigs" in SyncConfigManager:
if (active != null) {
config.setActive(active.booleanValue());
fActiveSyncConfigMap.put(project, config);
}
Won't this put all configs in the map, and thus the last config will become the active one?
2) Would it be better to store the active config as a separate value, rather than as a field of the SyncConfig, since only one config is supposed to be active at a time?
3) IMHO, SyncConfigManager should write changes immediately, instead of relying on "saveConfigs" to be called before Eclipse shutdown.
Thanks for the feedback. I've committed changes for these three issues. I've also added a pre-build hook so that #6 is now completed (though not tested until CDT support completed). I'm planning to look at the CDT plugins next. I have completed my planned CDT work. It is now possible to create a synchronized CDT project again. TODO: 1. Changes to the synchronized configurations for a CDT project are not handled. There is a listener in BuildConfigUtils that needs to be implemented. 2. It would probably make sense to modify the BuildRemotePropertiesPage so that it shows the connection between build configuration and the sync configuration. 3. CDT projects have not been extensively tested. John, would you be able to take a look at these? In particular it would be worth checking that BuildConfigUtils is doing the correct things with build configurations. John, can you let me know if you think it's stable enough to merge back into master? Thanks. I just tried from new_sync branch. I can create a C sync project and build and run. I say merge it back into master! I've been testing and so far it looks good, except for when I tried to convert a C project. I received the following NPE when I clicked on the project in the conversion wizard: java.lang.NullPointerException at org.eclipse.ptp.internal.rdt.sync.git.ui.GitParticipant.setProjectName(GitParticipant.java:268) at org.eclipse.ptp.internal.rdt.sync.cdt.ui.wizards.ConvertLocalToSyncProjectWizardPage.update(ConvertLocalToSyncProjectWizardPage.java:486) at org.eclipse.ptp.internal.rdt.sync.cdt.ui.wizards.ConvertLocalToSyncProjectWizardPage.access$0(ConvertLocalToSyncProjectWizardPage.java:482) at org.eclipse.ptp.internal.rdt.sync.cdt.ui.wizards.ConvertLocalToSyncProjectWizardPage$1.selectionChanged(ConvertLocalToSyncProjectWizardPage.java:176) at org.eclipse.jface.viewers.Viewer$2.run(Viewer.java:164) It is probably stable enough to merge back into master, though. Regular sync projects and CDT sync projects + building (all the normal things) seem to work fine. Good job! Thanks. I should have also added: 4. Conversion wizard needs to be tested/fixed. Merged to master. How should sync configs and CDT build configs be related? It seems to me that there is only one reason why they need to be related at all - the sync config indicates where the build should occur. I think the build should just always use the active sync config. It would be confusing if the build were to happen somewhere other than the users current selection. Implementing this policy by itself is enough... almost. Users would still have to remember to adjust both the sync config and build config when they want to switch to a different machine. So we should add a preference page where users can specify a default build config for each sync config. If the sync config changes, the build config automatically changes to its default. The user is free to change the build config, though, if they want to use something besides the default. On project creation, assuming the user selects one local and one remote toolchain, we can create two sync configs, local and remote, with the local having the local build config as its default and the remote having the remote build config as its default. Please let me know your thoughts. I can implement the changes once we decide what we should do. Yes, I think if the user selects a sync config then it should also select a default build config. If the user changes the build config separately then it should use whatever system the active sync config points to. Rather than adding yet another property page, I would suggest adding a customizable area at the bottom of the sync page that could be populated when you select a sync config from the list. The add config dialog could also have a customizable section so when a new sync config is added the corresponding build config could be selected. Both these areas could be populated by widgets contributed via an extension point. This would probably replace the current listeners, since you wouldn't need both. I've added a new extension point 'synchronizeProperties' that allows the synchronize properties page to be extended. There is a region at the bottom of the page that can be populated with any controls. The add button launches a wizard that is used to specify a new configuration. The extension point allows new wizard pages to be contributed to this dialog. I've done a prototype implementation for CDT projects, however it is not completely hooked up to the build configurations yet. If you could take a look and complete the implementation, it would be appreciated. Okay, it looks good. I will complete the implementation. I have also been working on moving the last piece of sync data (the sync config name) outside of CDT, since it is no longer needed. (We now always sync to the active sync config.) This allows us to remove a lot of the CDT-specific code. All of "BuildConfigUtils.java", except for the listeners, can be deleted. However, this breaks the current new project wizard, conversion wizard, and environment management page. (The latter two were already broken, I think, but would still compile.) I am now repairing and updating them. John, not sure how far you've got on this, but I'm wondering if you need to change things too much? Since the sync data is specific to CDT projects, does it matter if it's stored in a CDTspecific way? It might save you a bit of work to use the current method. Just a thought. Actually, the data that was still stored in CDT is useless, since we now always sync to the current sync config. (I should have said I was "removing" it, not just "moving" it.) So removing this code is helpful. The other code that still relies on these methods, and so would need to be fixed anyway, are now easily found. Just a quick update. I have almost finished implementing the new SynchronizedProperties extension plus everything else mentioned here, except for patching the conversion wizard and environment management page. Both of these do not compile, so I don't want to commit until they are fixed. If these last two tasks turn out to take awhile, though, I could create a separate branch so that you could look at the other changes. John, how is this progressing? I'd like to get it into M7 if possible, so synchronization is working again. Is there anything I can do to help? I plan to make a commit either today or tomorrow that will include a lot of changes, which I'll outline here. If you could then review and test it that would be very helpful. I just submitted several changes, which I broke into two commits to make it easier to review: First commit - fix sync-CDT interface * Removed "BuildConfigUtils.java" in cdt.core. * Created SyncConfigListenerCDT in cdt.core to listen for sync config changes. * Changed command launcher to always use the current active sync config. * Deleted the old build remote properties page. * Fixed the synchronize properties page. * SyncConfig now supports setting arbitrary properties, and this is used for storing build config information. * At startup, each config now has a local sync config as well as a remote config, where the local config allows the user to turn off sync'ing and switch to the local toolchain (local build config). Second commit - fix conversion wizard * Modified sync.ui project wizard to handle converting to a sync project as well. Note that this is only used to convert non-cdt projects to non-cdt sync projects. * Created new wizard in sync.cdt.ui to convert to Synchronized CDT projects. Tried to use as much of the CDT new project wizard as possible. * Changed cdt.ui.NewRemoteSyncProjectWizardOperation.java to handle both creating new projects and converting existing projects. * The CDT conversion wizard does not yet work fully. It seems to convert non-sync, CDT projects successfully, but not sync, non-CDT projects, or non-sync, non-CDT projects. It throws an exception when "Next" is pushed for the final page. NOTES * I hacked the environment management page so that it compiles, but it still needs to be fixed. * The Fortran wizard needs to be tested. I did a quick smoke test and it looks ok. Hopefully Beth will be able to do more testing before M7 next week. Thanks! Is there a way now to build a synchronized project locally without a running SSH daemon? It looks like an SSH connection is established in order to build locally. Would it be possible to add a toolbar entry to the main toolbar for switching the currently active synchronization configuration? Currently, you have to navigate through three context menus for that (Synchronize->Set Active->Remote system). An icon similar to the one for choosing the current build configuration would be great. (In reply to comment #29) > Would it be possible to add a toolbar entry to the main toolbar for switching > the currently active synchronization configuration? Currently, you have to > navigate through three context menus for that (Synchronize->Set Active->Remote > system). An icon similar to the one for choosing the current build configuration > would be great. Please open a separate bug. I'll take a look at this if I get a chance. Sure, the created bug is 408041. Conversion wizards have been committed to master and should now be fully functional. I removed the original, bulky conversion wizard to convert to a Synchronized CDT project and used a more lightweight approach. Here are some more details: 1) The synchronized project conversion wizard now has an extension point, "synchronizeWizardExtension", to add a page to the wizard. 2) A new wizard page exists in sync.cdt.ui for mapping sync configs to build configs. CDT appends this to the sync project conversion wizard for existing CDT projects. (This allows us to convert CDT projects to sync CDT projects.) 3) This page is also used by the new Sync CDT project wizard. 4) The normal CDT conversion wizard can be used to convert sync projects to Sync CDT projects, but users will have to configure default config mappings after conversion. 5) There is not a way currently to map non-sync, non-cdt projects to Sync CDT projects in a single step. 6) There is a new class, sync.ui.wizards.syncWizardDataCache, for exchanging data between sync wizard pages. (Setting up this communication is one of the trickiest parts of implementing a multi-page wizard.) 7) Unfortunately, the new page also appends itself to the other CDT new project wizards. The page has no effect, though, and can be ignored. This is a difficult problem to solve because of the complexity of the CDT wizard operation. Closing this as completed. |