Re: [Boost-build] CVS: boost/tools/build/v2/build generators.jam, 1.100, 1.101 targets.jam, 1.197, 1.198

From: Vladimir Prus (ghost_at_[hidden])
Date: 2006-08-06 14:50:12

Rene Rivera <grafikrobot <at> gmail.com> writes:

> 
> Vladimir Prus wrote:
> > On 8/2/06, Rene Rivera <grafik <at> users.sourceforge.net> wrote:
> 
> >> Skip targets that can't be generated like BBv1 does.
> > 
> > Just as a check -- did you run V2 testsuite before and after this 
> > change, to
> > verify it causes no regressions?
> 
> No, but that was because I didn't have the time. I'll get to that this 
> weekend.

Hmm, generally it's better to run regression tests *before* checkin. That's 
what I do, at the very least. 

> 
> > Index: generators.jam
> >> +    # Indicates if this generator can handle building the given target
> >> +    # combination.
> > 
> > I think this comment is not sufficient.
> 
> It seems as sufficient as some of the other commented methods in that 
> class. 

Are you saying those other comments are not sufficient too? That most likely 
true, but that does not mean we should not try to improve commenting for new 
checkings. Those other comments were probably written years ago, when I did not
realize how really important comments are.

> It says exactly what it means and nothing else. Perhaps comments 
> need to be added to where can-build is called to explain what effect it 
> has there.
> 
> > How returning false from this rule
> > is different from
> > returning nothing from the 'run' rule?
> 
> I have no clue. But then again the run rule doesn't say what happens 
> when it returns nothing.

I can fix that part; but that does not change the fact that 'can-build' is 
under-commented.

> 
> > I think there are some differences,
> > but the comment does not say what they are.
> 
> Perhaps... What do you think are the differences?

Well, returning nothing from 'run' method means the generator failed.
If all possible generators fail, then building a targets failed and an error 
is produced. As for 'can-build' returning false, you know better, but it seem 
to be some "half-failed" situation.

> 
> >> -rule find-viable-generators ( target-type : property-set )
> >> +rule find-viable-generators ( target-type : property-set : sources * )
> >> {
> >>     local key = $(target-type).$(property-set) ;
> >>     local l = $(.fv.$(key)) ;
> >>     if ! $(l)
> > 
> > 
> > You've added new parameter by you still key the .fv. catch on
> > target-type/property-set.
> > It means that if two invocation differ by 'source' the second one will 
> > reuse
> > results of the first.
> 
> OK, didn't notice there was a cache being use. May I suggest better 
> names for such variable. Perhaps something that has the word cache in 
> it, like "viable-generators-cache". I'll go fix the cache key problem 
> though 

I'm not sure about longer name -- won't that increase memory consumption of 
bjam?

> 
> >>  <at>  <at>  -733,8 +735,11  <at>  <at> 
> >>         for local p in $(all-property-sets)
> >>         {
> >>             local r = [ generate-really $(p) ] ;
> >> -            usage-requirements = [ $(usage-requirements).add $(r[1]) ] ;
> >> -            result += $(r[2-]) ;
> >> +            if $(r)
> >> +            {
> >> +                usage-requirements = [ $(usage-requirements).add $(r[1])
> >> ] ;
> >> +                result += $(r[2-]) ;
> >> +            }
> >>         }
> > 
> > So, this code allows a dependency to emit a warning, produce nothing, and
> > still go on. 
> 
> Yea, seems that way.
> 
> > It seems to be that if any dependency fails to build then
> > dependent's build should be aborted immediately.
> 
> If by "aborted" you mean that it should print a warning and skip 
> building the dependent's target? Then yes. How can a target be made to skip?

You need to modify abstract-target.generate so that it can return 'skipped' 
return value, and adjust all callers to act accordingly on 'skipped' value. 
That's gonna be a lot of work, though.

> 
> > I still don't understand why *failure* to build a target should become a
> > warning.
> 
> Because skipping unbuildable targets has been the expected behavior of 
> Boost.Build from the start. 

Well, we don't know if that's the behaviour users really want -- it was not 
documented like that, and nobody ever complained about either V1 behaviour or 
V2 behaviour.

> When users ask for something to get built it 
> should build as much as it can and only reports problems at the end. But 
> that's not really an argument, but an excuse. The real reason is that we 
> don't want to force artificial barriers on building what we think are 
> incorrect configurations. That just frustrates new users who might get 
> fed up with an uncooperative build system. And it frustrates experienced 
> users because it will prevent them from doing things that they know will 
> work.

That's an argument for disabling runtime-link=static check. We can discuss 
that decision, but your change does something different. It seem to make V2 
ignore all problems with building targets. So if no generators at all can't be 
found, you still get just warning. And dependents are still built. I don't 
think such hiding of problems is good. Imagine that V2 will do 30 min build of 
a large project, and then report about a problem, that will indicate the build 
properties are wrong, and you need yet another 30 min build.

> > In
> > general, if you ask generators.generate to produce target of type WHATEVER,
> > you expect it to do just that, or fail.
> 
> Returning nothing is a failure. It's just not aborting, without letting 
> you handle the error, as the code previously operated.

Hmm, I now recall that generators.generate are already supposed to return
empty list on failure.

> 
> > If you add a third option -- build
> > nothing -- you either need to use a special return code,
> 
> Returning and empty list, i.e. nothing, is a special return code. It 
> means the call was unable to satisfy your request.
> 
> > or check that all
> > callers will properly react to such "nothing" return.
> 
> Yes.
> 

Hmm, I start to think what you did could be done simpler. You can modify gcc 
linking generators, so that for runtime-link=static it returns nothing. Then, 
you can modify typed-targets so that when generators return nothing, warning 
is emitted instead of error (probably, just for Boost). Then, you won't need 
new generators method, as well as some of the changes you made.

This approach would simplify the code; though I still don't agree with target 
skipping approach.

Also, the problem with Python generators is likely to be problem present 
before your changes -- it should be checking that generators.generate return 
something.

- Volodya