Re: [jamboost] Re: [BBv2] two patches

From: Vladimir Prus (ghost_at_[hidden])
Date: 2003-11-05 02:43:28

pal.tw wrote:

> I describe the problem briefly. First, look at the following snippet.
> --------------------------------------------------------------------
> feature optimization : off speed space : propagated ;
> feature debug-symbols : on off : propagated ;
> feature runtime-debugging : on off : propagated ;
> feature inlining : off on full : propagated ;
>
> feature variant : : implicit composite propagated symmetric ;
> variant debug : <optimization>off <debug-symbols>on <inlining>off
> <runtime-debugging>on ;
> --------------------------------------------------------------------
>
> The 'variant' feature is not optional. So it has default value.
> But when "set-default variant : release ;",
> you will find it is not expanded as expected.

That's bad!

> Because expand-composites phase is prior to add-defaults phase,
> the default value of 'variant' has not been added yet.
> --------------------------------------------------------------------
> p = [ $(p).expand-composites ] ;
> p = [ $(p).add-defaults ] ;
> --------------------------------------------------------------------
>
> As to my patch. I have found it still have a problem myself.

Agh.. and I've found out that with the patch applied, some of the tests no 
longer pass.

> See the comment of the 'feature.jam' file.
>
> --------------------------------------------------------------------
> # Note: if there's there's ordinary feature F1 and composite feature
> # F2, which includes some value for F1, and both feature have default
> values,
> # then the default value of F1 will be added, not the value in F2.
> --------------------------------------------------------------------

Yea, that's the comment I looked at with wonder ;-)

> It seems that to set default to a composite feature is meaningless.
> Therefor let composite features alwalys be 'optional' might be a
> solution.
> Otherwise we had better consider their dependencies again.

I don't think this is the problem. 'expand-composites' expand them 
recursively. until it's left with simple properties. 

I though about it more, and came to the following reasoning:

Look at the code that you was patching. We start with a set of properties 
specified by the user. To me, it's quite clear that if user has specified 
<variant>release and it's composite features, we need to expand it.

Okay, so we expand it.

Then, we need to add default value. We add them. But we could have added 
default values of a composite feature. So we need to expand composites.

In essense, it mean you need one extra call to 'expand-composites', after 
'add-defaults'.

When I make this change, all tests still pass.

Now, where's the catch? The catch is that during composite expansion we don't 
add values which are already present. So, if default value of <variant> is 
release and default value of <debug-symbols> is on, we add both default 
value, and then forget about <debug-symbols>off which is present in 
<variant>release.

That's the primary problem. But, it's not so bad.

First, if I understand correctly, you're after <target-os> property, and it 
does not expand to any feature which already has a default value. So adding 
second 'expand-composites' might just work for you case. Maybe, you could try 
it?

Second, I don't think this problem is something Boost.Build should solve. In 
the case above, we can figure out that default value of <debug-symbols> is 
not clear, and ask the user to clarify his intentions -- e.g. by changing the 
default value of <debug-symbols>. 

- Volodya