Community
Participate
Working Groups
CDT has one builder CommonBuilder which delegates appropriately, depending on the build arguments and build configuration properties, to an appropriate build process. When #getRule() is called, we're not given any context of the build about to take place. As a result we don't know whether we can specify a null scheduling rule or not (because managed build depends on #getDelta() being consistent).
Created attachment 162771 [details] patch 1 Patch adding a getRule(int trigger, Map args) so the builder can work out which configuration is being built and whether a scheduling rule is needed.
Adding Martin & Toni as they've been tweaking builder API most recently. As we use CommonBuilder in CDT for delegating all builds, to work out what sort of rule to provide, we need to know what's being built. We can only get this is we have the build arguments...
The enhancement looks reasonable from my point of view and is completely backwards compatible.
I do see low risk and big value in this API addition, but is the value big enough for asking a PMC exception to the API freeze, which should have been M6? In the Javadocs for #getRule(int kind, Map args) I would like to see some more information what these args are, and where they can come from. Brief browsing seems to show that they can come from BuildCommand#getArguments(boolean), when build was invoked without arguments, or IProject#build(int trigger, String builderName, Map args, IProgressMonitor) if this is correct I think it should be specified in the Javadocs as examples where the args can come from (not asserting that this would be the only place the args can come from).
Also, why does this depend on bug 307094 and why does it block bug 289986 ? I see this API request independent of both issues, since a builder that's not very configurable won't need to know its arguments to return a scheduling rule - whereas a configurable builder will always need to know its arguments to return a scheduling rule. This is independent of race conditions or parallel work. Also, to assess whether it makes sense to ask for a PMC exception here, I'm wondering whether there is a workaround possible. CDT has a single "Common Builder" today, would it be possible to have CDT change and use multiple different builders for the different kinds of work to be done, such that it could benefit from relaxed scheduling rules even without this API addition?
(In reply to comment #5) > Also, why does this depend on bug 307094 and why does it block bug 289986 ? > > Also, to assess whether it makes sense to ask for a PMC exception here, I'm > wondering whether there is a workaround possible. CDT has a single "Common > Builder" today, would it be possible to have CDT change and use multiple > different builders for the different kinds of work to be done, such that it > could benefit from relaxed scheduling rules even without this API addition? CommonBuilders a bit of a beast, which I'm working on taming... bug 309714. The cause of the issue is that CDT has multiple configurations on a project. CommonBuilder builds a specific configuration (or more than one) based on the build arguments. If the configuration is 'managed' we can't relax scheduling rules currently, but we can if it's not. If there are different configurations there may be a mix of both. I don't think we need this urgently. As well as splitting the builder (which may not be too easy) is to stash state before the build, I'm happy to look at this again in 3.7.
Hm... I do see how different build configurations may result in a need for different scheduling rules. You say that you don't need this urgently, but on the other hand not having this in 3.6 will also block other client's adoption for a year. The API addition seems useful, safe and straightforward to me, so I tend to ask for the PMC exception. Szymon and John, what do you think? I could care for the logistics. BTW, if we end up adding #getRule(int,Map) now, I would also deprecate #getRule() with the same commit, since it has been made usable only recently. We could now instruct adopters to use the "extended" variant right away, such that the duplication of methods can eventually be removed. Keeping #getRule() now as the one and only method for relaxed scheduling rules would make deprecating it much harder in the future.
(In reply to comment #7) > Hm... I do see how different build configurations may result in a need for > different scheduling rules. Ah. Because each build configuration is entirely orthogonal. ConfigA may be managed (and needs #getDelta), ConfigB may be unmanaged (just runs make). The user can build both through one call to Project#build with appropriate arguments (we have UI to do this). If the user is just building ConfigB then getSchedulingRule can return null, if the user is building just ConfigA, or Config A and B in one go, it can't. > You say that you don't need this urgently, but on the other hand not having > this in 3.6 will also block other client's adoption for a year. Yes, good point.
Ok, so I can understand now why "Map args" is needed, but why do you think that the Builder needs to know the "int trigger" in order to decide what scheduling rule it should return?
(In reply to comment #9) > Ok, so I can understand now why "Map args" is needed, but why do you think that > the Builder needs to know the "int trigger" in order to decide what scheduling > rule it should return? Only for completeness. If it gets the same information as #build gets then there's no way someone can come along later and say "if only I had the build kind..."
The new API doesn't look risky, in that it is just a new method that will have a default implementation, so there is no binary compatibility problem. However, what about a builder that has overridden getRule() but hasn't adapted to the new getRule(int,Map) method)? It seems like we need to invoke both getRule methods in case the builder has only implemented one of them but not the other. My only real concern is whether these two new arguments are sufficient. Is there any risk that we will discover more contextual information required by builders and need to add another method later? James, have you prototyped using this proposed API in the CDT CommonBuilder to determine if it is enough? At first glance it seems that the new getRule() method is now consistent with the arguments for the build() method, which makes sense. Is there any other information a builder might need to determine the appropriate rule?
(In reply to comment #9) > Ok, so I can understand now why "Map args" is needed, but why do you think that > the Builder needs to know the "int trigger" in order to decide what scheduling > rule it should return? I can definitely imagine cases where the trigger could be useful. For example a builder might have much simpler work to perform on "clean" than on a "full" build.
(In reply to comment #11) > what about a builder that has overridden getRule() but hasn't adapted to the > new getRule(int,Map) method)? It seems like we need to invoke both getRule > methods in case the builder has only implemented one of them but not the other. The default implementation of #getRule(int, Map) calls #getRule(), so if users don't adapt to the new API, the old API is still called by default. > My only real concern is whether these two new arguments are sufficient. Is > there any risk that we will discover more contextual information required by > builders and need to add another method later? James, have you prototyped using > this proposed API in the CDT CommonBuilder to determine if it is enough? At > first glance it seems that the new getRule() method is now consistent with the > arguments for the build() method, which makes sense. Is there any other > information a builder might need to determine the appropriate rule? Yes I have, this particular bit of API is in the o.e.core.resources plugin in our product and it lets us successfully run makefile builds with a null scheduling rule. For building individual projects this works great. Where I still see problems with the API is in multi-project builds. The builders (&core.resources) doesn't currently have any knowledge of why the build is happening - did the user explicitly request a build of just the project, or the full workspace, or are we being built because a build configuration in another project references us? Common Builder explicitly builds references configurations in other projects, and this works well when we build one project, but it doesn't help us do the right thing when the user uses a UI BuildAction to build multiple-projects. This is a separate issue though, and there may be a clean solution without requiring changes to the core.resources. AFAICS this API change gives IncrementalProjectBuilder#getRule the same argument visibility that #build has. So, if in the future more context is needed there could be some addition mechanism to discover that, be it API, or wrapped in a build argument, or whatever.
We just discussed this on the Eclipse PMC call, and came to the conclusion that this API addition is worth approving if the new API would be leveraged in CDT Helios. James, do you see the CDT Helios release picking up this API change if we make it, such that at least Makefile builders could run without blocking editing?
(In reply to comment #14) > We just discussed this on the Eclipse PMC call, and came to the conclusion that > this API addition is worth approving if the new API would be leveraged in CDT > Helios. > > James, do you see the CDT Helios release picking up this API change if we make > it, such that at least Makefile builders could run without blocking editing? Certainly! That would be great and will make a huge difference to users using the CDT builders to build Makefile projects.
(In reply to comment #15) > Certainly! That would be great and will make a huge difference to users using > the CDT builders to build Makefile projects. Since it's going to be used in CDT Helios, we need pmc approval set on the bug.
(In reply to comment #16) > (In reply to comment #15) > > Certainly! That would be great and will make a huge difference to users using > > the CDT builders to build Makefile projects. > > Since it's going to be used in CDT Helios, we need pmc approval set on the bug. Martin, could you also commit the patch and deprecate the old #getRule method?
Sure! James could you please prepare an updated patch which - Updates Copyright headers - Updates the Javadoc description of args as per comment 4 - Deprecates #getRule() with notice to use the new method instead I think that after M7 we'll also want to announce removal of the old API as per http://wiki.eclipse.org/Eclipse/API_Central/Deprecation_Policy#Announcing_API_Removal but we'll want the new API established before we do that, so we should then track it with a separate bug.
(In reply to comment #18) > I think that after M7 we'll also want to announce removal of the old API as per > http://wiki.eclipse.org/Eclipse/API_Central/Deprecation_Policy#Announcing_API_Removal Sure it makes sense. I guess you will take care of it Martin ;)
Created attachment 165714 [details] Additional patch with deprecation and slight Javadoc updates I've ended up adding the Copyright comments, deprecation and some slight Javadoc polishing myself: - changed from "...outside the scope of the rule will not contain..." into "...outside the scope of the rule may not contain..." since if we move on with James' follow-up work, the #getDelta() may eventually include changes that happened after the build was started - added explicit "Subclasses may override this method." - changed from "...type of build being triggered. Valid values are..." into "...type of build being triggered. Valid values include..." since we may add other triggers in the future and we want getRule() implementers to be able and handle future triggers (typically pessimistic) - clarified the meaning of returning a null rule I decided against further clarifying the meaning of @param args (comment 4), since that would expose an implementation detail of the build manager - the same text for @param args is used everywhere else, too, so I thought that being more specific in this one place was unnecessary. I played with the idea of marking the new #getRule(int, Map) @noreference such that only the build manager were allowed to call it. Doing so may be a good idea since lifting the restriction later is always easier than adding it later, and I couldn't really see how anybody else than the build manager could make use of the method. What do others think?
Comment on attachment 162771 [details] patch 1 Committed. Keeping the bug open until we have some comments regarding @noreference for #getRule(int, Map).
(In reply to comment #19) Created bug 310072 to track the request for deleting the old #getRule() API. http://wiki.eclipse.org/Eclipse/API_Central/Deprecation_Policy#Announcing_API_Removal
(In reply to comment #20) I like the patch. > - added explicit "Subclasses may override this method." I don't think we need to explicitly say that. If there is no @nooverride, people are just allowed to override it. > I played with the idea of marking the new #getRule(int, Map) @noreference such > that only the build manager were allowed to call it. Doing so may be a good idea > since lifting the restriction later is always easier than adding it later, and I > couldn't really see how anybody else than the build manager could make use of > the method. What do others think? That's true that this method was intended to be used by the build manager, not to be called by other clients. Hence I think @noreference makes sense here.
Created attachment 165720 [details] tweak for comment A couple quick comments: - There's a discrepancy between the 'int trigger' on getRule and 'int kind'. This is tied to where in the BuildManager implementation #getRule is currently called... If we relax rules per builder (as bug 307098), this might change. Perhaps we could rename this to kind now so as not to confused integrators and better match #build? - "If the builder rule is non-<code>null</code> it must be "contained" in the workspace root rule...." It's a shame to have this requirement. I can see it's needed to cope with old callers of the API that run it with a WR rule held. However it does block the useful use-case of a builder requesting a rule which contains both IResources to be modified, and some domain specific scheduling rule. Given that, for relaxed scheduling rules to be useful, callers of the Project#build should not hold the WR rule (as noted in note 2), could this be relaxed to: * If the builder rule is non-<code>null</code> it must "contain" resource rules for * any IResources the builder job intends to modify. * I.e. {@link ISchedulingRule#contains(ISchedulingRule)} must return * <code>true</code> when called on the builder's scheduling rule, with the resource to be * modified as an argument.
(In reply to comment #23) Thanks Szymon - added @noreference. I'd like to keep the "Subclasses may override this method" in there, because - In contrast to @noreference it says what clients *may* do - There is the subtle difference between "overriding" and "extending" a method - If in doubt, it's always good to be explicit about what we might think is implicitly clear But I can remove it if you feel strongly. (In reply to comment #24) > There's a discrepancy between the 'int trigger' on getRule and 'int kind'. I checked and found that in all API we seem to be talking about "int kind" whereas in most internal implementation we talk about "int trigger". Since this is API, I agree that "int kind" is better. > "If the builder rule is non-<code>null</code> it must be "contained" in > the workspace root rule...." As of today, I think the restriction is necessary. It's easy to lift the restriction in the future, especially if the method is @noreference. But for now I think we need it. I also think that there's a workaround (rather than use a completely domain-specific scheduling rule, return some non-existing resource that your builder creates only in memory but not on disk and only your builder knows about). I've committed the "@noreference" and "int kind" changes. Marking FIXED.
(In reply to comment #25) > I'd like to keep the "Subclasses may override this method" in there, because > - In contrast to @noreference it says what clients *may* do > - There is the subtle difference between "overriding" and "extending" a method > - If in doubt, it's always good to be explicit about what we might think > is implicitly clear Sure, having extra clarification is good. Thanks Martin :)
