Bugzilla – Full Text Bug Listing
| Summary: | [services][launch] Allow to extend ServicesLaunchSequence | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Marc Khouzam <marc.khouzam> | ||||||||||
| Component: | cdt-debug-dsf-gdb | Assignee: | Nobody - feel free to take it <nobody> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Marc Khouzam <marc.khouzam> | ||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | abeerbagul, cdtdoug, elaskavaia.cdt, nobody, pawel.1.piech, vladimir.prus | ||||||||||
| Version: | 8.0 | Flags: | nobody:
iplog-
marc.khouzam: review+ |
||||||||||
| Target Milestone: | 8.0 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Linux | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Marc Khouzam
Created attachment 180190 [details]
Prototype solution
This patch does the following:
1- Merge ServicesLaunchSequence and ShutdownSequence and replace them with a nwe GdbServicesSequence class. Since whenever we add a service to ServicesLaunchSequence we must also add it to ShutdownSequence, I felt it would be a good reminder to have the two sequences as a single class.
2- Use the new DSF ReflectionSequence to make the new GdbServicesSequence easy to override.
3- Add GdbLaunch.getServicesSequence() to allow people to override the services sequence.
The problems I ran across, which explain some of the code changes are:
- Unlike FinalLaunchSequence, the ShutdownSequence is used from the launch instead of the launch delegate. Therefore, it must be overridden in GdbLaunch instead of GdbLaunchDelegate, which is why the new getServicesSequence() method is in GdbLaunch
- ServicesLaunchSequence took an IProgressMonitor as a parameter, while ServicesLaunchSequence to a RequestMonitor. To be able to merge the two into a single class, I used a RequestMonitorWithProgress, which can be used as either an IProgressMonitor or a RequestMonitor.
- in an attempt to make the adding of a service even easier, I added code that takes the launchSequence step order and automatically reverses it to create the shutdownSequence step order.
Using this patch, everything seems to be working as expected. However, I didn't actually try to extend the new class, so if somewhat wans to try it and give feedback, it would be good.
To provide a custom services sequence using this patch, I need to extend GdbLaunch class and overwrite "getServicesSequence()". To replace GdbLaunch by my own launch class I need to overwrite "getLaunch()" in GdbLaunchDelegate class. "GdbLaunchDelegate::getLaunch()" apart of creating an instance of GdbLaunch also initializes some private fields: "isNonStopSession" and "fIsPostMortemTracingSession". In my case these fields will not be initialized. The workaround is to call super.getLaunch() and ignore the returned launch, but it's a hack. Can we move the initialization to another method, to "launch() for instance"? (In reply to comment #2) > To provide a custom services sequence using this patch, I need to extend > GdbLaunch class and overwrite "getServicesSequence()". > To replace GdbLaunch by my own launch class I need to overwrite "getLaunch()" > in GdbLaunchDelegate class. > "GdbLaunchDelegate::getLaunch()" apart of creating an instance of GdbLaunch > also initializes some private fields: "isNonStopSession" and > "fIsPostMortemTracingSession". In my case these fields will not be initialized. > The workaround is to call super.getLaunch() and ignore the returned launch, but > it's a hack. > Can we move the initialization to another method, to "launch() for instance"? I don't see why those vars are initialized in getLaunch()... Can't we just initialize them right before they are being used? Meaning, in launchDebugSession()? (In reply to comment #3) > I don't see why those vars are initialized in getLaunch()... > Can't we just initialize them right before they are being used? Meaning, in > launchDebugSession()? There are some other minor issues. I'll submit a patch. Created attachment 186578 [details]
Modified patch.
GdbServicesSequence:
- removed unnecessary @since tags from the methods, the class has @since tag
- added protected getters for fLaunch, fOperation and fSession
GdbLaunchDelegate:
- moved the initialization of fIsNonStopSession and fIsPostMortemTracingSession to launchDebugSession()
- made getSourceLocator() protected
Marc, can you please review the changes? (In reply to comment #6) > Marc, can you please review the changes? I didn't test it, but the patch looks good to me. Feel free to check it in if you want. We'll have to document this in http://wiki.eclipse.org/CDT/User/NewIn80#DSF-GDB_2 and http://wiki.eclipse.org/CDT/cdt-debug-dsf-gdb-extensibility (the second requested improvement) I have some questions here: 1. Why should ShutdownSequence explicitly code steps to shutdown every known service. I mean, DsfSession keeps a list of services, so it should be possible to enumerate them, and dynamically create Step to showdown every single one of them. In this way, the class would not have to be updated for every new step, and it would not matter whether it's used from GdbLaunch or GdbLaunchDelegate. 2. If the above is true, then can we deal with ServicesLaunchSeqeunce the same way we deal with FinalLaunchSequence? 3. Why do we even have register new service inside ServicesLaunchSeqeunce? Can it be registered as the first step of FinalLaunchSequence? (In reply to comment #8) > I have some questions here: > > 1. Why should ShutdownSequence explicitly code steps to shutdown every known > service. I mean, DsfSession keeps a list of services, Does it? I don't see it. > so it should be possible > to enumerate them, and dynamically create Step to showdown every single one of > them. In this way, the class would not have to be updated for every new step, > and it would not matter whether it's used from GdbLaunch or GdbLaunchDelegate. That would be nice. The important thing would be to respect the reverse order, but we can do that using IDsfService#getStartupNumber() > 2. If the above is true, then can we deal with ServicesLaunchSeqeunce the same > way we deal with FinalLaunchSequence? probably, but is #1 true? > 3. Why do we even have register new service inside ServicesLaunchSeqeunce? Can > it be registered as the first step of FinalLaunchSequence? The two sequences used to be a single one at the very start. However, if you look in GdbLaunchDelegate, there are a couple of things to do between creating the services and running the FLS. That is why we split it. > > 1. Why should ShutdownSequence explicitly code steps to shutdown every > > known service. I mean, DsfSession keeps a list of services, > > Does it? I don't see it. Well, indirectly. ShutdownSequence uses fTracker.getService right now. However, fTracker appears to hold two maps, fServiceReferences and fServices, which together appear to be suitable for iterating over all services in a given DsfSession. Now, DsfServicesTracker does not expose any method to actually iterate over services, but can that be added. > > > so it should be possible > > to enumerate them, and dynamically create Step to showdown every single > > one of them. In this way, the class would not have to be updated for > > every new step, and it would not matter whether it's used from GdbLaunch > > or GdbLaunchDelegate. > > That would be nice. The important thing would be to respect the reverse > order, but we can do that using IDsfService#getStartupNumber() > > > 2. If the above is true, then can we deal with ServicesLaunchSeqeunce the > > same way we deal with FinalLaunchSequence? > > probably, but is #1 true? > > > 3. Why do we even have register new service inside > > ServicesLaunchSeqeunce? Can it be registered as the first step of > > FinalLaunchSequence? > > The two sequences used to be a single one at the very start. However, if > you look in GdbLaunchDelegate, there are a couple of things to do between > creating the services and running the FLS. That is why we split it. Unfortunately, it's not documented why those things should go in between, and what services should be already installed when those two things are done. (In reply to comment #10) > > > 1. Why should ShutdownSequence explicitly code steps to shutdown every > > > known service. I mean, DsfSession keeps a list of services, > > > > Does it? I don't see it. > > Well, indirectly. ShutdownSequence uses fTracker.getService right now. > However, fTracker appears to hold two maps, fServiceReferences and > fServices, which together appear to be suitable for iterating over all > services in a given DsfSession. Now, DsfServicesTracker does not > expose any method to actually iterate over services, but can that be added. If there is a good way to do it, I am all for it. > > The two sequences used to be a single one at the very start. However, if > > you look in GdbLaunchDelegate, there are a couple of things to do between > > creating the services and running the FLS. That is why we split it. > > Unfortunately, it's not documented why those things should go in between, > and what services should be already installed when those two things are > done. Not very well, you are right. And I don't totally remember the details. They are in a bugzilla somewhere. I know that adding a process to a launch would hang if we did in on the DSF executor, so it had to be done outside the sequence (although there are other ways around it that I did not know at the time). But besides all that, I think it is a better design to handle the services separately than the FLS. What's the deadline for adding new API? I'm afraid with all these discussions we will end up without a proper way of registering custom services in CDT 8. (In reply to comment #12) > What's the deadline for adding new API? I'm afraid with all these discussions > we will end up without a proper way of registering custom services in CDT 8. I think M6 is API freeze (March ~18) and M7 is feature freeze (May ~6). I'm ok with the last patch posted if it works well for you guys. So, I suggest that Mikhail commits his patch now to make sure we have a solution. If Volodya has time to come up with a better solution before API freeze, we can always commit it if we all agree it is worth it. Is that ok? (In reply to comment #13) > (In reply to comment #12) > > What's the deadline for adding new API? I'm afraid with all these discussions > > we will end up without a proper way of registering custom services in CDT 8. > > I think M6 is API freeze (March ~18) and M7 is feature freeze (May ~6). > > I'm ok with the last patch posted if it works well for you guys. So, I suggest > that Mikhail commits his patch now to make sure we have a solution. If Volodya > has time to come up with a better solution before API freeze, we can always > commit it if we all agree it is worth it. > > Is that ok? Sounds good to me. I'll commit the patch. Except it's not my patch, it's your's. (In reply to comment #14) > Sounds good to me. I'll commit the patch. Except it's not my patch, it's > your's. Well I started it and you finished it. If you don't mind dealing with it, that would save me time :-) I still think that the patch in the current form is not ideal, and it forces non-ideal design on extenders. Therefore, why should we actually rush it in (and presumably have it stay forever for API compatibility reasons)? (In reply to comment #16) > I still think that the patch in the current form is not ideal, and it forces > non-ideal design on extenders. Therefore, why should we actually rush it in > (and presumably have it stay forever for API compatibility reasons)? Re. compatibility. In fact, this patch gets rid of some old public classes (SerivesLaunchSequence and ShutdownLaunchSequence), so "staying forever" objection is not correct. Let's be more specific. The following API changes are proposed in the patch: 1. GdbLaunch::getServicesSequence() 2. GdbLaunchDelegate::getSourceLocator() 3. SerivesLaunchSequence and ShutdownLaunchSequence are replaced GdbServicesSequence I don't think there is any objection for #2. It is required to allow subclassing of GdbLaunch. Re.#1 and #3. This API simply indicates that any launch has a set of services, nothing more. It doesn't expose implementation details, doesn't force clients to implement something new. What's wrong with it? Hmm. Could it be clarified whether DSF API is required to be backward-compatible at this point? My primary problem is with #1. It means that if you wish to customize the set of services you need to go a longer road of first derivign a new class from GdbLaunch and then overriding getServicesSequence, and then making GdbLaunchDelegate create that new class. It seems that adding getServicesSequence to GdbLaunchDelegate is an easier design. And I suppose that if we add getServicesSequence to GdbLaunch now, then adding this method to GdbLaunchDelegate later will either create even more confusion, or will require that GdbLaunch.getSerivcesSequence be removed, which is bad for backward compatibility. Given that (i) API freeze is a month from now and (ii) we actually haven't arrive to the final patch that was supposed to use this upstream feature, I would suggest that we don't rush this change. How about we arrive at a finished patch we'll use internally, and then I can update this upstream one? (In reply to comment #18) > Given that (i) API freeze is a month from now and (ii) we actually haven't > arrive to the final patch that was supposed to use this upstream feature, I > would suggest that we don't rush this change. > > How about we arrive at a finished patch we'll use internally, and then I can > update this upstream one? What's wrong with committing the current solution now, and you updating upstream with your new solution exactly as you would have done (except that we'd also revert the change of this bug)? It's a tad more work, but it makes sure we have a solution, in case you run into delays. (In reply to comment #18) > Hmm. Could it be clarified whether DSF API is required to be > backward-compatible at this point? > In general, not necessarily for major versions. We try to keep backward compatibility but if it is not possible we can always make changes in major versions. Ok, let take some time and revisit the patch. When we (in the Xtensa Xplorer IDE) create a subclass of GDBLaunchDelegate and a subclass of GDBLaunch, my intention is just to return the object of the GDBLaunch subclass from the getLaunch() method. I would not like to override the entire getLaunch() method, as this forces me to override the launch object initialization sequence also. If the getLaunch() method can call a createLaunch() method to get the launch object, and the createLaunch() method is protected, then we can just override createLaunch(), return an object of GDBLaunch subclass, and have the getLaunch() method do the initialization of the launch object. If I want to override the initialization sequence of the launch object, override getLaunch() as well as createLaunch(). The same applies to GDBLaunchDelegate.getSourceLocator(). This should delegate object creation to a protected createSourceLocator(), which just returns a new DSFSourceLocator. Subclasses of GDBLaunchDelegate can just override createSourceLocator, or override getSourceLocator() too if they want to change the initialization sequence of the sourcelocator object. Am attaching the modified patch with addition of two methods: createLaunch() createSourceLocator() and calling these from getLaunch and getSourceLocator respectively. Created attachment 189393 [details]
Modified patch with createLaunch() and createSourceLocator()
I've investigated the possibility of using the service tracker to perform the shutdown of registered services and here is the conclusion. First of all, the order in which services are shut down is important. Services are registered with BundleContext which doesn't keep the registration order, at least I couldn't find anything. DsfServiceTracker is definitely not a place to keep track of the registration order - the instances of it are created on fly everywhere. The only place to store the ordered list of registered services is GdbLaunch which requires a new API for it. The current patch can be simplified but a new method in GdbLaunch is still required. Comments? (In reply to comment #23) > I've investigated the possibility of using the service tracker to perform the > shutdown of registered services and here is the conclusion. > First of all, the order in which services are shut down is important. > Services are registered with BundleContext which doesn't keep the registration > order, at least I couldn't find anything. AbstractDsfService#initialize stores a count of the order. You can get it using IDsfService#getStartupNumber() If you have the entire list of services, I guess you could sort them using that. But I haven't looked at how easy it is. (In reply to comment #24) > AbstractDsfService#initialize stores a count of the order. You can get it > using IDsfService#getStartupNumber() > > If you have the entire list of services, I guess you could sort them using > that. But I haven't looked at how easy it is. We still need the list of services registered by the session. I can't find a way to get it directly from BundleContext. Do you know how to do it? It is possible to add ServiceListener to BundleContext and keep track of the registered services. So it's doable, but looks like a hack. (In reply to comment #25) > (In reply to comment #24) > > AbstractDsfService#initialize stores a count of the order. You can get it > > using IDsfService#getStartupNumber() > > > > If you have the entire list of services, I guess you could sort them using > > that. But I haven't looked at how easy it is. > > We still need the list of services registered by the session. I can't find a > way to get it directly from BundleContext. Do you know how to do it? Every DSF service, when it registers, automatically also registers using the name IDsfService. This is done in AbstractDsfService#register (line 174). DsfServicesTracker does not seem to allow you to get a reference to more than one service, but if you can go to the BundleContext directly, you could use that name to get all the services. I'm worried about the filtering though. Can we use a filter that will give us all the services for one session, and only those? > It is possible to add ServiceListener to BundleContext and keep track of the > registered services. So it's doable, but looks like a hack. BundleContext? That in OSGI. Did you mean something else? (In reply to comment #26) > Every DSF service, when it registers, automatically also registers using the > name IDsfService. This is done in AbstractDsfService#register (line 174). > DsfServicesTracker does not seem to allow you to get a reference to more than > one service, but if you can go to the BundleContext directly, you could use > that name to get all the services. > > I'm worried about the filtering though. Can we use a filter that will give us > all the services for one session, and only those? > I don't know how filtering works. But IDsfService has getSession() method. We can use it to filter the result, can't we? > > It is possible to add ServiceListener to BundleContext and keep track of the > > registered services. So it's doable, but looks like a hack. > > BundleContext? That in OSGI. Did you mean something else? GdbPlugin provides access to BundleContext, all services are registered with it. (In reply to comment #27) > I don't know how filtering works. But IDsfService has getSession() method. We > can use it to filter the result, can't we? I'm not familiar with the filtering either. But when looking at DsfServicesTracker finds a service, we can see that it uses filtering to make sure it finds the service of the right session. I guess we can do the same thing. > > > It is possible to add ServiceListener to BundleContext and keep track of the > > > registered services. So it's doable, but looks like a hack. > > > > BundleContext? That in OSGI. Did you mean something else? > > GdbPlugin provides access to BundleContext, all services are registered with > it. I was just confused when you said "add ServiceListener to BundleContext". Doesn't that mean modifying BundleContext? (In reply to comment #28) > I was just confused when you said "add ServiceListener to BundleContext". > Doesn't that mean modifying BundleContext? BundleContext has addServiceListener() and removeServiceListener(). Sorry, I should have said "register a listener with BundleContext". SUMMARY of the below: I'm suggesting moving the ServicesLaunchSequence to the ServicesFactory. === I got to wonder if we can't make this effort even better. Would it be worth allowing to extend DSF-GDB without having to create a new launch configuration type? If we could avoid having to override GdbLaunchDelegate, then an extender could re-use the existing CDT launch configuration types. This would basically allow to change DSF-GDB without the user needing to be aware of it. I believe CDI allows this, as pointed out in bug 334747. I haven't actually extended DSF-GDB so I may not realize some of the issues. Here is what I'm thinking, please let me know if I missed an important detail. As part of Bug 338769 I plan to commit a change that will move the overriding of the FinalLaunchSequence from GdbLaunchDelegate to the IGDBControl service. This made we wonder what would be left that an extender really needs to override in GdbLaunchDelegate? I see three major things: 1- ability to override GdbLaunch (GdbLaunchDelegate#getLaunch) 2- ability to override the serviceLaunchSequence 3- ability to override the ServiceFactory (GdbLaunchDelegate#newServiceFactory) I'm not sure about #1. The GdbLaunch does not have much stuff in it and I'm not sure people actually need to override it. For #2, that is what this bug discusses. As for #3, it is important I believe. This is the way to override the existing services. So, I was thinking we could provide an extension point to allow a plugin to add a new service factory. Actually, we probably need a NewServicesFactoryFactory, which would create the services factory based on the different launch attributes. But even if we have a way to inject a custom ServicesFactory, we still need a way to get a new ServicesLaunchSequence to trigger any new services supported by the factory. So, it made me think that the servicesLaunchSequence should be part of the ServiceFactory itself. Even without any extension point, it would make overriding the ServicesLaunchSequence easier. I'm not sure about the ShutdownSequence, but this may work out also, if we are able to get the automatic shutdown proposed by Volodya. Opinions? (In reply to comment #30) > SUMMARY of the below: I'm suggesting moving the ServicesLaunchSequence to the > ServicesFactory. > > === > > I got to wonder if we can't make this effort even better. > > Would it be worth allowing to extend DSF-GDB without having to create a new > launch configuration type? If we could avoid having to override > GdbLaunchDelegate, then an extender could re-use the existing CDT launch > configuration types. This would basically allow to change DSF-GDB without the > user needing to be aware of it. I believe CDI allows this, as pointed out in > bug 334747. > CDI is different. From a custom view clients can get hold of the current gdb session and do whatever they want. I really doubt the whole service injection stuff without overriding GdbLaunchDelegate. The problem is not how to register a service but when. Another problem is when to shut it down. GdbLaunch.shutdownSession() is called when 1. the session is terminated (ICommandControlShutdownDMEvent is dispatched) 2. the launch is canceled 3. the workbench with active sessions is being shut down Writing handlers for all these events just to shutdown a service is not a simple task. It would be much easier to let DSF/GDB do it instead. > I haven't actually extended DSF-GDB so I may not realize some of the issues. > Here is what I'm thinking, please let me know if I missed an important detail. > > As part of Bug 338769 I plan to commit a change that will move the overriding > of the FinalLaunchSequence from GdbLaunchDelegate to the IGDBControl service. > This made we wonder what would be left that an extender really needs to > override in GdbLaunchDelegate? I see three major things: > > 1- ability to override GdbLaunch (GdbLaunchDelegate#getLaunch) > 2- ability to override the serviceLaunchSequence > 3- ability to override the ServiceFactory (GdbLaunchDelegate#newServiceFactory) > > I'm not sure about #1. The GdbLaunch does not have much stuff in it and I'm > not sure people actually need to override it. > I disagree. Launch extensions are often used to store data acquired at launch time. You save the shutdown sequence, for instance. > For #2, that is what this bug discusses. > > As for #3, it is important I believe. This is the way to override the existing > services. So, I was thinking we could provide an extension point to allow a > plugin to add a new service factory. Actually, we probably need a > NewServicesFactoryFactory, which would create the services factory based on the > different launch attributes. > > But even if we have a way to inject a custom ServicesFactory, we still need a > way to get a new ServicesLaunchSequence to trigger any new services supported > by the factory. So, it made me think that the servicesLaunchSequence should be > part of the ServiceFactory itself. Even without any extension point, it would > make overriding the ServicesLaunchSequence easier. I'm not sure about the > ShutdownSequence, but this may work out also, if we are able to get the > automatic shutdown proposed by Volodya. > > Opinions? Re. Volodya's proposal. I don't see how to get the list of registered services. Do you know how to do it? (In reply to comment #31) > CDI is different. From a custom view clients can get hold of the current gdb > session and do whatever they want. > I really doubt the whole service injection stuff without overriding > GdbLaunchDelegate. I have to admit I didn't try it. It was just an idea. > The problem is not how to register a service but when. Right. That is why I was thinking it would be safer to override the entire service creation. Meaning both the ServicesFactory and the ServiceLaunchSequence, by merging the two into one class. > Another problem is when to shut it down. GdbLaunch.shutdownSession() is called > when > 1. the session is terminated (ICommandControlShutdownDMEvent is dispatched) > 2. the launch is canceled > 3. the workbench with active sessions is being shut down > > Writing handlers for all these events just to shutdown a service is not a > simple task. It would be much easier to let DSF/GDB do it instead. But with the auto shutdown idea, couldn't we have DSF-GDB do that for the extender? > > This made we wonder what would be left that an extender really needs to > > override in GdbLaunchDelegate? I see three major things: > > > > 1- ability to override GdbLaunch (GdbLaunchDelegate#getLaunch) > > 2- ability to override the serviceLaunchSequence > > 3- ability to override the ServiceFactory (GdbLaunchDelegate#newServiceFactory) > > > > I'm not sure about #1. The GdbLaunch does not have much stuff in it and I'm > > not sure people actually need to override it. > > I disagree. Launch extensions are often used to store data acquired at launch > time. You save the shutdown sequence, for instance. I wasn't sure about overriding the launch. Maybe that is something we can (later on) find a good way to do. > Re. Volodya's proposal. I don't see how to get the list of registered services. > Do you know how to do it? This seems to work (just a prototype): try { ServiceReference<?>[] services = GdbUIPlugin.getBundleContext().getServiceReferences( IDsfService.class.getName(), ("(" + IDsfService.PROP_SESSION_ID + "=" + id + ")").intern()); } catch (InvalidSyntaxException e) {} (In reply to comment #32) > I wasn't sure about overriding the launch. Maybe that is something we can > (later on) find a good way to do. > Abeer modified the patch to allow GdbLaunch extensions. > > Re. Volodya's proposal. I don't see how to get the list of registered services. > > Do you know how to do it? > > This seems to work (just a prototype): > > try { > ServiceReference<?>[] services = > GdbUIPlugin.getBundleContext().getServiceReferences( > IDsfService.class.getName(), > ("(" + IDsfService.PROP_SESSION_ID + "=" + id + ")").intern()); > } catch (InvalidSyntaxException e) {} Thanks, I'll try this and if it works will submit a patch. Created attachment 190711 [details]
New implementation of shutdown sequence.
Marc, attached is new implementation of ShutdownSequence which acquires all services registered for the given session from BundleContext (as you suggested), sorts them by start number and generates corresponding shutdown steps.
Please take a look. It's not urgent: there is no API changes.
(In reply to comment #34) > Created attachment 190711 [details] > New implementation of shutdown sequence. > > Marc, attached is new implementation of ShutdownSequence which acquires all > services registered for the given session from BundleContext (as you > suggested), sorts them by start number and generates corresponding shutdown > steps. I skimmed over it and it seems fine. It's nice that we don't have to worry about overriding (or changing) the ShutdownSequence anymore. > Please take a look. It's not urgent: there is no API changes. What do you want to do about overriding the ServicesLaunchSequence? That has API changes. (In reply to comment #35) > What do you want to do about overriding the ServicesLaunchSequence? That has > API changes. I don't think we need it. With the new ShutdownSequence we can register services somewhere else. I do it in my FinalLaunchSequence. Let's wait until somebody else comes up with a new use case. I would suggest to separate Abeer's part in a separate patch to allow extending GdbLaunch. (In reply to comment #36) > (In reply to comment #35) > > What do you want to do about overriding the ServicesLaunchSequence? That has > > API changes. > > I don't think we need it. With the new ShutdownSequence we can register > services somewhere else. I do it in my FinalLaunchSequence. Let's wait until > somebody else comes up with a new use case. > I would suggest to separate Abeer's part in a separate patch to allow extending > GdbLaunch. That is a good point. +1 Mikhail, I like your last patch very much. Thanks! Do I understand correctly that there are no further DSF changes that we immediately need? (In reply to comment #38) > Mikhail, > > I like your last patch very much. Thanks! Do I understand correctly that there > are no further DSF changes that we immediately need? That's correct. The original purpose of this enhancement was to allow registration of custom services by debugger extensions. With this patch clients can register a service (in FinalLaunchSequence, for instance) without thinking of how to shut it down. Created https://bugs.eclipse.org/bugs/show_bug.cgi?id=339550 and copied Abeer's patch. Committed the shutdown sequence patch to HEAD. Marc, please review it. *** cdt cvs genie on behalf of mkhodjai *** Bug 326951: [services][launch] Allow to extend ServicesLaunchSequence. [*] ShutdownSequence.java 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/ShutdownSequence.java?root=Tools_Project&r1=1.3&r2=1.4 (In reply to comment #40) > Created https://bugs.eclipse.org/bugs/show_bug.cgi?id=339550 and copied Abeer's > patch. > Committed the shutdown sequence patch to HEAD. > Marc, please review it. Very nice and elegant. Thanks for the patch Mikhail and thanks for the idea Volodya. |