Community
Participate
Working Groups
+++ This bug was initially created as a clone of Bug #305858 +++ > Apart from the NPE there seems to be no good reason why null should not be > allowed. There's a problem with relaxing scheduling rules, and allowing some builders to return null. If the project contains a combination of builders: -(1) at least one returns null (and expects to modify resources) -(2) at least one specified a scheduling rule The build will fail, as the build MultiRule representing (2) doesn't contain the resources which will be modified by the builder (1). The problem with the API is that builders (1) & (2) don't know anything about each other. Builder (1) is using the API to be as flexible as possible -- don't lock anything -- and Builder (2) wants some subset of the tree to be locked. The MultiRule generated isn't compatible with builder (1). Problems occurred building the selected resources. Errors running builder 'CDT Builder' on project 'SrpDownlink'. Attempted to beginRule: P/p1, does not match outer scope rule: MultiRule[F/p1/Debug] Attempted to beginRule: P/p1, does not match outer scope rule: MultiRule[F/p1/Debug] Attempted to beginRule: P/p1, does not match outer scope rule: MultiRule[F/p1/Debug] Attempted to beginRule: P/p1, does not match outer scope rule: MultiRule[F/p1/Debug] Errors running builder 'Scanner Configuration Builder' on project 'SrpDownlink'. Attempted to beginRule: R/, does not match outer scope rule: MultiRule[F/p1/Debug]
A simple way to handle this case is to fall back to the default workspace root rule, ie. if a builder returns a null rule and another builder a non-null rule, be pessimistic and use the workspace root.
(In reply to comment #1) > A simple way to handle this case is to fall back to the default workspace root > rule, ie. if a builder returns a null rule and another builder a non-null rule, > be pessimistic and use the workspace root. My thoughts exactly -- that's the easiest fix for 3.6. Hopefully this can be relaxed further for 3.7, but the above seems like a good compromise for the moment.
Created attachment 162866 [details] Fall back to workspace root if rule-less builder is not "alone" Thanks James. This is my proposed solution.
(In reply to comment #3) > Thanks James. This is my proposed solution. Thanks Toni, you beat me to it. I've verified that this patch fixes the issue in comment 0 and it looks good to me. On a related note: Unless we can get a fix in for bug 306822, we can't make this work better for CDT 7 external make targets. I'm guessing it's too late in the day to get that reviewed?
+1 for Anton's fix, but I'll let Szymon review it because he added the builder rule support.
I think we should be fixing this for M7 one way or the other. Would the patch attached to bug 307098 also fix this issue, but in a more sophisticated way?
(In reply to comment #6) > I think we should be fixing this for M7 one way or the other. Yes, I think this patch should be committed. > Would the patch attached to bug 307098 also fix this issue, but in a more > sophisticated way? The patch there takes a completely separate approach, yes. It runs each builder with its scheduling rule only (being careful to ensure #getDeltas etc. is respected). The current build system uses a MultiRule of all builders to be run and this patch fixes the issue there.
James, could you write a regression test for the issue?
(In reply to comment #8) > James, could you write a regression test for the issue? In test 1 on bug 307098, I believe testTwoBuildersRunInOneBuild() should test just this condition. Note that the fix there runs each build with its own scheduling rule rather than creating a multi-rule of both builders' rules. Just use the commented out assert rather than the existing assert.
(In reply to comment #9) > In test 1 on bug 307098, I believe testTwoBuildersRunInOneBuild() should test > just this condition. I'll factor this out and attach it to this bug later when I get a chance.
(In reply to comment #10) > (In reply to comment #9) > > In test 1 on bug 307098, I believe testTwoBuildersRunInOneBuild() should test > > just this condition. > > I'll factor this out and attach it to this bug later when I get a chance. I checked the fix in to HEAD. James, I'm leaving the bug open till you have a test to commit. Thanks.
Created attachment 165800 [details] test 1 This patch contains 2 of the test from bug 307098. It adds a new suite 'RelaxedSchedRuleBuilderTest' with: - testBasicRelaxedSchedulingRules() + tests that a null-scheduling rule build works and is run with a null rule - testTwoBuildersRunInOneBuild() + tests this bug: two builders can be run in the same build operation, one with a project rule, one with a null rule. The assertion is flexible in that, if we run builders with just their requested scheduling rule in the future the test will continue to pass. Other changes: - extend TestBuilder with a 'call-back' mechanism. RelSchedRuleTests registers to be notified on #getRule and #build. Default implementation continues to behave as before. - uses the new getRule(int,Map) API - new empty Delta Builder for testing purposes All existing builder tests continue to pass.
I think, we can reuse EmptyDeltaBuilder class and just register it twice in plugin.xml. Otherwise, it looks good :-)
(In reply to comment #13) > I think, we can reuse EmptyDeltaBuilder class and just register it twice in > plugin.xml. Otherwise, it looks good :-) Yep, I agree! (There was originally more stuff in there, but it moved to TestBuilder.)
I've committed the fix to HEAD. We can tweak it later, if really needed. Thanks James.
One issue is that the BuildManager creates a single rule for all builders and has an API to do it. In practice, there is no strong reason for using a common rule for all builders in scheduling. Each builder should just require its own rule, and no super rule should be automatically used nor created.
Discussion continues in bug 538462, it seems like further changes made this fix not required anymore and undesired with latest development.
