Community
Participate
Working Groups
+++ This bug was initially created as a clone of Bug #330360 +++ Currently, the post-build refresh is run with its own null progress monitor. This has a lot of negative effects. - For a large project, or one on a slow filesystem, the refresh can take a long time (for large remote projects, it might take an hour or more). Not reporting progress on a long-running operation such as this is unacceptable. In fact, the contract for IncrementalProjectBuilder says "All builders should report their progress and honor cancel requests in a timely manner." - If the user decides that the build is taking too long and cancels it, they are still stuck waiting for the refresh. It happens regardless of whether the build is canceled or not. The reasoning behind this was that there still might be binaries/generated files that will show up in the project after a partial build, but given how long some refreshes can take, the user may not wish to wait for this. Add in the progress reporting problem, and the user tends to think that nothing is being canceled and that the IDE has hung up. Again, "All builders should report their progress and honor cancel requests in a timely manner." - The user also cannot cancel the refresh while it is in progress. There are a couple of options here. Simplest is to make the post-build refresh use the same progress monitor as the build itself, and not launch the refresh if the progress monitor has already been canceled. This would mean that if you cancel a build, you won't see newly generated files from the partial build, unless you refresh manually. That seems a lesser evil than not reporting progress though. It might also be possible to move the refresh to a separate Job with its own progress reporting. That way the artifacts of a partial build would still show up unless the user also cancels the refresh Job. It also means though that someone that wishes to cancel out of a build entirely has two things to cancel instead of one. Thoughts?
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.
