Bugzilla – Full Text Bug Listing
| Summary: | remoteMakeBuilder's post-build refresh should report progress and be cancelable | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] PTP | Reporter: | Chris Recoskie <recoskie> | ||||||
| Component: | RDT | Assignee: | Chris Recoskie <recoskie> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | major | ||||||||
| Priority: | P3 | CC: | cdtdoug, jamesblackburn+eclipse, ptp-inbox, vivkong | ||||||
| Version: | 4.0.5 | ||||||||
| Target Milestone: | 4.0.6 | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Chris Recoskie
See discussion in Bug 330360. We're going to fix this by putting the refresh in its own Job. (In reply to comment #1) > See discussion in Bug 330360. We're going to fix this by putting the refresh > in its own Job. How will that help? The workspace will still be locked for the entire duration of the Job... (In reply to comment #2) > (In reply to comment #1) > > See discussion in Bug 330360. We're going to fix this by putting the refresh > > in its own Job. > How will that help? The workspace will still be locked for the entire duration > of the Job... The refresh Job is scheduled on the resources to refresh. Since the build has the project locked, the refresh job executes after the build finishes. These semantics only really work for exteral builds. I think the internal buildr might get messed up if we interefered with its refresh logic. (In reply to comment #3) > (In reply to comment #2) > > (In reply to comment #1) > > > See discussion in Bug 330360. We're going to fix this by putting the refresh > > > in its own Job. > > How will that help? The workspace will still be locked for the entire duration > > of the Job... > > The refresh Job is scheduled on the resources to refresh. Since the build has > the project locked, the refresh job executes after the build finishes. Ok. But when you do: IResource.refresh() in your refresh job, that part of the resource tree will be locked until the refresh has finished. The platform refresh job on the other hand breaks the refresh into smaller pieces. > These semantics only really work for exteral builds. I think the internal > buildr might get messed up if we interefered with its refresh logic. For CDT external builds, we already run the build with a null scheduling rule -- i.e. the workspace is completely unlocked until the refresh starts at the end locking the project and below... (In reply to comment #4) > Ok. But when you do: > IResource.refresh() in your refresh job, that part of the resource tree will be > locked until the refresh has finished. The platform refresh job on the other > hand breaks the refresh into smaller pieces. The platform RefreshJob is internal. Doesn't it only handle auto-refresh? Or are you just saying that I should make my new refresh job break up the refreshes? In this case the refresh is on the entire project so it's not really divisible. > For CDT external builds, we already run the build with a null scheduling rule > -- i.e. the workspace is completely unlocked until the refresh starts at the > end locking the project and below... Are you sure the project isn't locked during the build? I would think it's dangerous if it isn't. Then you could have people modifying the project resources as you try to build them. Bad things tend to happen when you do that. (In reply to comment #5) > The platform RefreshJob is internal. Doesn't it only handle auto-refresh? Or > are you just saying that I should make my new refresh job break up the > refreshes? In this case the refresh is on the entire project so it's not > really divisible. You're right it currently only handles auto-refresh. It's would be a very small patch to add IResource.asynchRefresh() plumbed into this. If you don't break the refresh up then won't you just trade off the build blocking edits with a refresh job blocking edits? > Are you sure the project isn't locked during the build? I would think it's > dangerous if it isn't. Then you could have people modifying the project > resources as you try to build them. Bad things tend to happen when you do > that. Why is it dangerous? People have used emacs and vi for years without locking the source while they build. The build system already checks timestamps (or in more advanced cases hashes like scons) to rebuild the object when the source changes. Note we still lock the workspace for managedbuild, as we can't guarantee to do the right thing in this case. I have a two line patch which makes the delta work in this case too though. As long as we always ensure that .o's are newer than the source files then it's ok. In the case of managedbuild the platform can tell us which files have been modified since the *start* of the previous build. (In reply to comment #6) > You're right it currently only handles auto-refresh. It's would be a very small > patch to add IResource.asynchRefresh() plumbed into this. But I need something that works with Helios, and IResource.asyncRefresh() doesn't exist there. Does it exist in Indigo yet? > If you don't break the refresh up then won't you just trade off the build > blocking edits with a refresh job blocking edits? Yes, but it's no worse than it was. This bug is only trying to handle the progress reporting and cancelability. I'm not trying to address *what* is refreshed in this bug. I'm only changing the timing slightly by moving it to a job so that the refresh can still happen even if the build is canceled. The same resouces (i.e. the whole project) are locked and refreshed. > Why is it dangerous? People have used emacs and vi for years without locking > the source while they build. The build system already checks timestamps (or in > more advanced cases hashes like scons) to rebuild the object when the source > changes. As far as I know, make only does one dependency scan up front. If you make changes in the middle of the build, they won't result in new build steps kicking off. I'm more worried about someone trying to write to a file while the compiler is reading it. The world won't end, but it could result in a failed build with error feedback that is very cryptic. That's all I mean by dangerous. Maybe it was a bit of hyperbole :-) > Note we still lock the workspace for managedbuild, as we can't guarantee to do > the right thing in this case. I have a two line patch which makes the delta > work in this case too though. As long as we always ensure that .o's are newer > than the source files then it's ok. In the case of managedbuild the platform > can tell us which files have been modified since the *start* of the previous > build. That's best discussed on Bug #330360 I think issues are getting a bit confused because the clone duplicated the entire cc: list... I'm going to prune down the list and try to keep this bug for discussion and tracking of the RDT changes. Basically just remove the CDT build inbox. Created attachment 183485 [details]
patch for discussion
Attaching a patch that moves the refresh to a Job.
The approach may change later, but I'm checking this in for PTP 4.0.6 to meet our product deadlines. Patch has been applied to ptp_4_0 and HEAD. Comment on attachment 183485 [details]
patch for discussion
Somehow not all the changes were included in the patch. Doh.
Created attachment 183493 [details]
patch
Patch with all changes.
|