From 74eb969e436aef64957378b4918d3eaa3f81edb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jurko=20Gospodneti=C4=87?= Date: Thu, 28 Aug 2008 19:20:27 +0000 Subject: [PATCH] Fixed a Boost Jam bug causing it to sometimes trigger actions depending on targets that have not been built yet. Test case included. Updated related code comments. Bug was happening when we had a multifile action that got triggered to build its non-initial target. Then while that action was being executed all the other targets were reporting as 'already built' and were getting used by other actions prematurely. Quick-fixed by making all targets built by a single action list each other as 'included' causing anything else depending on any of these targets to automatically depend on all the others in the group as well. The solution is not perfect as it might have some unexpected interactions with other uses of 'included' targets and now if any target in a group is not up to date then all of them will be rebuilt even if actually did not need the target that was up to date. On the other hand this should be a really rare use case as it would require the one target in a group to be up to date and be needed while another in the same group (i.e. built by the same action) to not be up to date. [SVN r48426] --- historic/jam/src/compile.c | 45 ++++++++++++----- historic/jam/src/rules.c | 18 ++++++- historic/jam/src/rules.h | 1 + ...s.jam => parallel_multifile_actions_1.jam} | 4 +- .../jam/test/parallel_multifile_actions_2.jam | 49 +++++++++++++++++++ historic/jam/test/test.jam | 3 +- 6 files changed, 105 insertions(+), 15 deletions(-) rename historic/jam/test/{parallel_multifile_actions.jam => parallel_multifile_actions_1.jam} (95%) create mode 100644 historic/jam/test/parallel_multifile_actions_2.jam diff --git a/historic/jam/src/compile.c b/historic/jam/src/compile.c index 3a5e896f9..b6386467e 100644 --- a/historic/jam/src/compile.c +++ b/historic/jam/src/compile.c @@ -1060,22 +1060,45 @@ evaluate_rule( action->targets = targetlist( (TARGETS *)0, lol_get( frame->args, 0 ) ); action->sources = targetlist( (TARGETS *)0, lol_get( frame->args, 1 ) ); - /* Make targets[1,N-1] depend on targets[0], to describe the multply - generated targets for the rule. Do it with includes, to reflect - non-build dependency. */ + /* If we have a group of targets all being built using the same action + * then we must not allow any of them to be used as sources unless they + * had all already been built in the first place or their joined action + * has had a chance to finish its work and build all of them anew. + * + * Without this it might be possible, in case of a multi-process build, + * for their action, triggered by buiding one of the targets, to still + * be running when another target in the group reports as done in order + * to avoid triggering the same action again and gets used prematurely. + * + * As a quick-fix to achieve this effect we make all the targets list + * each other as 'included targets'. More precisely, we mark the first + * listed target as including all the other targets in the list and vice + * versa. This makes anyone depending on any of those targets implicitly + * depend on all of them, thus making sure none of those targets can be + * used as sources until all of them have been built. Note that direct + * dependencies could not have been used due to the 'circular + * dependency' issue. + * + * TODO: Although the current implementation solves the problem of one + * of the targets getting used before its action completes its work it + * also forces the action to run whenever any of the targets in the + * group is not up to date even though some of them might not actually + * be used by the targets being built. We should see how we can + * correctly recognize such cases and use that to avoid running the + * action if possible and not rebuild targets not actually depending on + * targets that are not up to date. + * + * TODO: Using the 'include' feature might have side-effects due to + * interaction with the actual 'inclusion scanning' system. This should + * be checked. + */ if ( action->targets ) { TARGET * t0 = action->targets->target; for ( t = action->targets->next; t; t = t->next ) { - TARGET * tn = t->target; - if ( !tn->includes ) - { - tn->includes = copytarget( tn ); - tn->includes->original_target = tn; - } - tn = tn->includes; - tn->depends = targetentry( tn->depends, t0 ); + add_include( t->target, t0 ); + add_include( t0, t->target ); } } diff --git a/historic/jam/src/rules.c b/historic/jam/src/rules.c index 1ac888947..809162e2e 100644 --- a/historic/jam/src/rules.c +++ b/historic/jam/src/rules.c @@ -60,7 +60,23 @@ struct _located_target { static struct hash *located_targets = 0; - +/* + * add_include() - adds the 'included' TARGET to the list of targets included by + * the 'including' TARGET. Such targets are modeled as dependencies of the + * internal include node belonging to the 'including' TARGET. + */ +void add_include( TARGET * including, TARGET * included ) +{ + TARGET * internal; + if ( !including->includes ) + { + including->includes = copytarget( including ); + including->includes->original_target = including; + } + internal = including->includes; + internal->depends = targetentry( internal->depends, included ); +} + /* * enter_rule() - return pointer to RULE, creating it if necessary in diff --git a/historic/jam/src/rules.h b/historic/jam/src/rules.h index 563808252..9f8c04b63 100644 --- a/historic/jam/src/rules.h +++ b/historic/jam/src/rules.h @@ -230,6 +230,7 @@ struct _target { char* failed; } ; +void add_include( TARGET * including, TARGET * included ); RULE *bindrule( char *rulename, module_t* ); RULE* import_rule( RULE* source, module_t* m, char* name ); diff --git a/historic/jam/test/parallel_multifile_actions.jam b/historic/jam/test/parallel_multifile_actions_1.jam similarity index 95% rename from historic/jam/test/parallel_multifile_actions.jam rename to historic/jam/test/parallel_multifile_actions_1.jam index e6734da92..d11de7a12 100644 --- a/historic/jam/test/parallel_multifile_actions.jam +++ b/historic/jam/test/parallel_multifile_actions_1.jam @@ -4,7 +4,7 @@ if ! $(BJAM_SUBTEST) { - ECHO --- Testing -jN parallel execution of multi-file actions... ; + ECHO --- Testing -jN parallel execution of multi-file actions - 1... ; assert "...found 6 targets... ...updating 4 targets... @@ -16,7 +16,7 @@ if ! $(BJAM_SUBTEST) .use.2 u2.user 004 ...updated 4 targets... -" : (==) : [ SHELL "\"$(ARGV[1])\" -f parallel_multifile_actions.jam -sBJAM_SUBTEST=1 -j2" ] ; +" : (==) : [ SHELL "\"$(ARGV[1])\" -f parallel_multifile_actions_1.jam -sBJAM_SUBTEST=1 -j2" ] ; } else { diff --git a/historic/jam/test/parallel_multifile_actions_2.jam b/historic/jam/test/parallel_multifile_actions_2.jam new file mode 100644 index 000000000..a85cf63fd --- /dev/null +++ b/historic/jam/test/parallel_multifile_actions_2.jam @@ -0,0 +1,49 @@ +# Copyright 2008 Jurko Gospodnetic, Vladimir Prus +# Distributed under the Boost Software License, Version 1.0. +# (See accompanying file LICENSE_1_0.txt or http://www.boost.org/LICENSE_1_0.txt) + +# Added to guard against a bug causing targets to be used before they +# themselves have finished building. This used to happen for targets built by a +# multi-file action that got triggered by another target, except when the target +# triggering the action was the first one in the list of targets produced by +# that action. +# +# Example: +# When target A and target B were declared as created by a single action with +# A being the first one listed, and target B triggered running that action then +# while the action was still running, target A was already reporting as being +# built causing other targets depending on target A to be built prematurely. + +if ! $(BJAM_SUBTEST) +{ + ECHO --- Testing -jN parallel execution of multi-file actions - 2... ; + + assert "...found 4 targets... +...updating 3 targets... +link dll +001 - linked +install installed_dll +002 - installed +...updated 3 targets... +" : (==) : [ SHELL "\"$(ARGV[1])\" -f parallel_multifile_actions_2.jam -sBJAM_SUBTEST=1 -j2" ] ; +} +else +{ + actions link + { + sleep 1 + echo 001 - linked + } + + link dll lib ; + + actions install + { + echo 002 - installed + } + + install installed_dll : dll ; + DEPENDS installed_dll : dll ; + + DEPENDS all : lib installed_dll ; +} diff --git a/historic/jam/test/test.jam b/historic/jam/test/test.jam index d1b57acf6..1d1e3d61a 100644 --- a/historic/jam/test/test.jam +++ b/historic/jam/test/test.jam @@ -58,7 +58,8 @@ include option_d2.jam ; include option_l.jam ; include option_n.jam ; include parallel_actions.jam ; -include parallel_multifile_actions.jam ; +include parallel_multifile_actions_1.jam ; +include parallel_multifile_actions_2.jam ; include stress_var_expand.jam ; include target_var.jam ; include var_expand.jam ;