Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 364191 - Use synchronization for non-CDT projects
Summary: Use synchronization for non-CDT projects
Status: RESOLVED FIXED
Alias: None
Product: PTP
Classification: Tools
Component: RDT.sync (show other bugs)
Version: 7.0   Edit
Hardware: Macintosh Mac OS X
: P3 normal (vote)
Target Milestone: 7.0   Edit
Assignee: Greg Watson CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 408496
Blocks:
  Show dependency tree
 
Reported: 2011-11-18 14:39 EST by Greg Watson CLA
Modified: 2013-06-06 13:01 EDT (History)
4 users (show)

See Also:


Attachments
sync patch (182.24 KB, patch)
2013-04-09 13:55 EDT, Greg Watson CLA
no flags Details | Diff
updated patch including new plugins (364.50 KB, patch)
2013-04-09 14:01 EDT, Greg Watson CLA
no flags Details | Diff
updated patch (653.38 KB, patch)
2013-04-09 14:23 EDT, Greg Watson CLA
no flags Details | Diff
updated patch to make it easier to apply (476.61 KB, patch)
2013-04-09 15:34 EDT, Greg Watson CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Watson CLA 2011-11-18 14:39:59 EST
It should be possible to use the synchronization framework for non-CDT projects. The main issue with the current implementation is probably the integration with the CDT build configurations. Can this be abstracted so that other types of build configurations could be supported?
Comment 1 Greg Watson CLA 2013-04-09 13:55:36 EDT
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.
Comment 2 Greg Watson CLA 2013-04-09 14:01:37 EDT
Created attachment 229523 [details]
updated patch including new plugins
Comment 3 Greg Watson CLA 2013-04-09 14:23:23 EDT
Created attachment 229525 [details]
updated patch
Comment 4 Greg Watson CLA 2013-04-09 15:34:47 EDT
Created attachment 229531 [details]
updated patch to make it easier to apply
Comment 5 Greg Watson CLA 2013-04-10 09:32:09 EDT
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.
Comment 6 Greg Watson CLA 2013-04-10 19:40:14 EDT
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.
Comment 7 Greg Watson CLA 2013-04-12 19:54:36 EDT
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.
Comment 8 Greg Watson CLA 2013-04-15 15:26:12 EDT
9) Added SyncProjectWidget that encapsulates the local and remote project location information.
Comment 9 John Eblen CLA 2013-04-15 17:00:08 EDT
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.
Comment 10 Greg Watson CLA 2013-04-16 09:09:01 EDT
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.
Comment 11 Greg Watson CLA 2013-04-16 17:14:44 EDT
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.
Comment 12 Greg Watson CLA 2013-04-17 12:22:19 EDT
John, can you let me know if you think it's stable enough to merge back into master? Thanks.
Comment 13 Beth Tibbitts CLA 2013-04-17 15:00:06 EDT
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!
Comment 14 John Eblen CLA 2013-04-17 15:23:03 EDT
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!
Comment 15 Greg Watson CLA 2013-04-17 16:11:59 EDT
Thanks. 

I should have also added:

4. Conversion wizard needs to be tested/fixed.
Comment 16 Greg Watson CLA 2013-04-17 16:32:17 EDT
Merged to master.
Comment 17 John Eblen CLA 2013-04-18 15:28:31 EDT
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.
Comment 18 Greg Watson CLA 2013-04-19 16:39:12 EDT
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.
Comment 19 Greg Watson CLA 2013-04-24 11:45:25 EDT
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.
Comment 20 John Eblen CLA 2013-04-24 13:11:28 EDT
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.
Comment 21 Greg Watson CLA 2013-04-24 14:11:11 EDT
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.
Comment 22 John Eblen CLA 2013-04-24 14:43:24 EDT
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.
Comment 23 John Eblen CLA 2013-04-26 17:23:30 EDT
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.
Comment 24 Greg Watson CLA 2013-05-02 08:56:26 EDT
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?
Comment 25 John Eblen CLA 2013-05-02 11:25:37 EDT
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.
Comment 26 John Eblen CLA 2013-05-03 18:06:10 EDT
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.
Comment 27 Greg Watson CLA 2013-05-04 08:46:24 EDT
I did a quick smoke test and it looks ok. Hopefully Beth will be able to do more testing before M7 next week. Thanks!
Comment 28 Carsten Karbach CLA 2013-05-07 09:58:14 EDT
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.
Comment 29 Carsten Karbach CLA 2013-05-07 10:11:13 EDT
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.
Comment 30 Greg Watson CLA 2013-05-14 11:14:30 EDT
(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.
Comment 31 Carsten Karbach CLA 2013-05-14 12:21:53 EDT
Sure, the created bug is 408041.
Comment 32 John Eblen CLA 2013-05-20 14:58:49 EDT
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.
Comment 33 Greg Watson CLA 2013-06-06 13:01:12 EDT
Closing this as completed.