Re: [jamboost] Refactoring progress report

From: Vladimir Prus (ghost_at_[hidden])
Date: 2003-05-13 00:25:54

Ali Azarbayejani wrote:
> David and I have been discussing a number of issues relating to
> refactoring the BBv2 codebase and we thought it would be a good time
> to summarize some of them so you can see what we're planning and have
> an opportunity for early feedback.

It's quite a long list of proposed changes! I'll try to answer them all, but 
for now will focus only on the most important ones, with more comments to 
follow.

> o Dynamic Type Checking
>
> A major problem for comprehension throughout is lack of variable
> types and inconsistent naming. For example, a "property set" can
> take three forms: a list, a path, or an instance of
> object(property-set). Functions that take specific forms usually
> use "properties" or "property-set" as the argument name, often
> resulting in some confusion over which form the argument should be.
> (See, e.g. feature.split, property.as-path).
>
> Another related problem is that arguments are generally named to
> reflect the *type* of the argument, often resulting in confusion as
> to the *role* of the argument.
>
> The proposal is to introduce dynamic type checking. David can give
> more details, but the resulting syntax will allow the confusing
>
> rule split ( property-set )
> rule as-path ( properties * : feature-space ? )
>
> to be written more explicitly as something like
>
> rule <property-list> path-to-list ( <property-path> input-path )
> rule <property-path> list-to-path ( <property-list> input-list :
> feature-space ? )
>
> or
>
> rule [property-list] path-to-list ( [property-path] input-path )
> rule [property-path] list-to-path ( [property-list] input-list :
> feature-space ? )
>
> (David likes the square brackets because they don't require a shift.
> I don't care about the shift, very slightly prefer the angle
> brackets.)
>
> (Note: I changed "feature.split" to "path-to-list" and
> "property.as-path" to "list-to-path" to emphasize the complementary
> nature of the two functions. More on feature/property refactoring
> see below.)
>
> This type of readability problem is widespread throughout the
> system. The new syntax is backward compatible (arguments and rules
> do not require types) and the syntax can be introduced for better
> readability before actually implementing the dynamic type checking.
> The dynamic type checking would work by registering a type-checking
> function for each type. This would be done automatically for
> classes. The actual type checking could be turned off to avoid any
> possible performance hit.
>
>
>
> o Explicit Importing
>
> Currently "import foo" in a module doesn't guarantee that the module
> is dependent upon foo and the absence of "import foo" doesn't
> guarantee that the module is NOT dependent on foo. The latter is
> because "import foo" in another module makes foo rules available
> globally.
> ...
> The solution is to modify "modules.import" to make foo rules
> available only to the module that is importing them.

+1

> o Class Declaration
>
> Each class declaration requires a "rule" declaration followed by a
> "class" declaration, making it difficult to see at a glance whether
> a rule is a regular rule or a class definition. Short of a core jam
> modification to allow a more obvious declaration, it may be possible
> to reduce confusion by allowing the placing of the "class"
> declaration just before the "rule" declaration. David plans to
> explore this possibility. We're not sure there is an easy solution.

I'd prefer more serious redesign of classes to tweaking them a bit. What I 
don't like is 
- the declaration is not obvious
- creation of instances involves executing lots of code, with uncertain 
performance.

Some time ago, I was thinking about a different way to declare classes:

module my_class 
{
rule __init__ ( self arg1 : arg2 )
{
$(self).arg1 = $(arg1) ; 
.....
}

rule do ( self )
{
ECHO $($(self).arg1) ;
}
}

To create instance you just generate unique id for class and call __init__ 
method --- which is much simpler than the current approach. The syntax for 
using classes stays the same --- provided a new builtin facility is 
introduced --- bound rules, which have implicit argument (self) automatically 
prepended on invocation. I even implemented that few months ago, and moved
some classes to the new scheme, but had no time to push that further. 

I'm not sure this is precisely the way to go, but probably classes are used 
extensively enough to have better core support?

> o Option plug-ins
>
> Module "bootstrap.jam" is conceptually low-level, but depends upon
> "doc.jam", which results in a great deal of unwanted dependency on
> text-processing modules. This is a minor architectural problem.
>
> Actually, bootstrap.jam depends only on the parsing of command line
> options representing help requests. One way to decouple bootstrap
> from doc and provide a general mechanism for extending command line
> options is to provide a plug-in architecture for command-line
> options. The "help" module would use the option plug-in technique
> to express that it wants to handle certain command line options and
> then exit. Other modules might do the same, or handle command line
> options and continue. Thus, doc is decoupled from bootstrap and we
> have a new command-line-option extension feature.
>
>
> o Layers (tools, build, core)
>
> A coarse layering of the 55 current .jam files results in the
> following three conceptually and structurally useful layers. Note,
> however, that refactoring will result in creating, deleting, and
> renaming modules. Some current modules contain material that
> crosses layers and needs to be separated somehow.

Some time ago I've drawn a package diagram for V2 (after Ali posted his 
version). I attach it just in case. Looks like basically, they layers are the 
same as proposed now, except that my one is more detailed.

> - core ----------------------------------------------------------

Just a naming issue: "core" sounds like "the most important part" for me. In 
fact, it's just a set of utilities. But I may be wrong with english.

> The "core" layer is a library of bootstrap and core "BBv2 language"
> constructs and utilities. 

Yes, you give different meaning to "core".

> The "build" layer contains the
> generalized build system logic, with no specific knowledge of how to
> build anything in particular. The "tools" layer contains specific
> functionality of tools. (Note that "builtin" does not really need
> to be built in to the "build" layer...it basically consists of
> generalized compiler and linker tool functionality and should
> therefore be in the "tools" layer...we plan to refactor this into
> modules called perhaps "compiler" and "linker".)
>
> In general,
>
> o BBv2 users interact only with the "tools" layer,
> o BBv2 extenders interact with the "build" layer, but typically
> only a few modules are meant for normal extensions.
> o only BBv2 developers interact with the "core" layer,
>
> There is a strict dependency relationship: each layer depends only
> upon lower layers: tools->build->core.

I hope you don't mean a layer can't use modules from the same layer? It's not 
clear from the above.

> Note, however, that within the "core" layer specifically, there is
> no reasonable way to cleanly layer modules within the layer. For
> example, modules->assert,errors and assert->modules,
> errors->modules...these circular module dependencies are unavoidable
> (in any reasonable manner). However, the modules within the "build"
> layer can be (with some re-factoring) made to be strictly layered
> and this is a goal of the refactoring. Generally, the modules in
> "tools" are independent resulting in a flat module layering in
> "tools".

Yes, that's right. Circular depencendies in "build" are not desirable and 
"tools" simply depend on modules in "build".

> We propose eventual re-structuring of the BBv2 directory to have
> three main subdirectories "tools", "build", and "core" containing
> the functionality described above, after refactoring. This would
> result in import statements like:
>
> import tools/gcc ;
> import build/type ;
> import core/os ;
>
> We realize the proposed names result in source-tree paths like
> "boost/tools/build/tools" and "boost/tools/build/build", which is
> kind of ugly. Any suggestions?

No, unfortunately. But separation of modules to directories must be done. 
Tools, especially, ask for a directory if their own --- there can be possibly 
a lot of tools.

> o Refactoring feature.jam, property.jam
>
> We spent a lot of time specifically on modules "feature",
> "property", and "property-set", as they are the lowest-level modules
> in the "build" layer and the right place to start. Some
> observations include
>
> - There is no real separation between the concept of "feature" and
> the concept of "property". Features exist for the purpose of
> properties. Thus the artificial division between modules "feature"
> and "property", whatever the original motivation, doesn't really
> work, is slightly confusing or misleading, and should be eliminated.

Don't expect opposition from me, after all, "property" module was created by 
Dave ;-) Seriously, the bound between "feature" and "property" is indeed 
illusive.

> - However, there does seem to be a need for more than one module for
> the feature/property content. Module "features" should contain the
> core concepts and simple manipulations of single features,
> subfeatures, implicit features, single properties, and property
> lists. A second module called something with the meaning
> "feature-processing" might be introduced to contain some of the
> higher-level manipulations like "minimize", conditional properties,
> and most of the other contents of the current "property" module.

This sounds reasonable.

> - Class "property-map" should be in its own file.

+/- 0

> - The introduction of class "property-set" at some point in the
> development seems to have created some confusion with terminology in
> earlier-written code, but it seems as though it serves two necessary
> purposes: (1) provide a single object for manipulating what would
> otherwise be a list and (2) caching results of certain operations
> for performance reasons. Volodya, perhaps you can confirm that
> these are the reasons for its existence? 

Yes. (2) was the primary reason. (1) is a reason too --- property-set is much
easier to work with that bare list, and this gives additional optimization 
possibilities, as well.

> This module should basically stay as it is.

OK.

> - Throughout these three modules and BBv2 in general there is much
> inconsistency in terminology and naming. The Dynamic Type Checking
> proposal above will allow us to write clearer code. Some
> refactoring will re-group functions in a more meaningful way and
> re-name them. For example, to convert a property-list to a
> property-path you call "property.as-path", but to do the opposite,
> you call "feature.split"; these functions should be in the same
> module and named in a way that expresses their complementary nature.

They are not complimentaty!

[ feature.split [ property.as-path $(p) ] ] != $(p) 

"as-path" produces as compact representation of property set as possible. For 
example, if you have "debug" and "<optimization>off" in property set, then 
only "debug" will show up in "path", because it already contains the second 
property. This means that if you decide to rename 'as-path' you must be 
carefull about the name.

> o Refactoring classes
>
> We propose that each class have its own file.

This sounds too radical for me. We'll end up with a big number of modules, 
some of that are so small that just don't carry their own weight. I'll see if 
I can come up with better argumentation...

> o "Abstract Targets" vs. "Virtual Targets"
>
> One of the most confusing aspects of the system is the existence of
> "abstract-targets", which "generate" "virtual-targets", which
> "actualize" "Jam targets". There are class heirarchies for
> "abstract-targets" and "virtual-targets".
>
> Our observations and recommendations:
>
> - Eliminate the "abstract-target" base class and resulting
> heirarchy. The derived classes ("project-target", "main-target",
> "basic-target") are not really related and are never used
> polymorphically.

I'm not so sure. Imagine you have a command line like

bjam @project1 @project2/x

where project1 is... a project, and project2/x is a target in project2. The 
build-system module would lookup both target id and get two abstract targets, 
one of which is project-target and another is main-target. They have 
polymorphic 'generate' method, which allows generate both of them. Not sure 
that 'project.lookup' will return project target right now, but the use case 
above is perfectly reasonable.

So, I think project-target and main-target are related. As to basic-target --- 
in fact, I don't see a possibility to use it polymorphically together with 
main-target, so it probably does not belong to this hierrarchy. The only
dependency on 'abstract-target' is 'full-name' method that 'basic-target' 
uses.

> - Rename "project-target" -> something like "project-spec" or simply
> "project". Possibly absorb all the functionality of existing
> modules "project" and "project-root" into this class...they are
> circularly dependent anyway!

I'm -1 on "project-spec". You can't say that "project-spec" contains main 
target, or can be generated. OTOH, "project" can do both. I'm fine with this 
name.

> - Rename "main-target" -> something like "main-target-spec" ??? It
> doesn't actually represent a "target"...it respresents a set of
> formulas, any one or more of which can be used to create variants of
> a target with this id. Anyway, it is a user-level concept and
> confusing with respect to internal notions of "target".

I'm -1 on *-spec for the same reason as above. '*-spec' is like a textual 
specification, without any behaviour. I also think 'main-target' is fine. 
This term is inherited from V1 and still makes sense to me. I think it's 
natural for user to think that there are projects with main targets in them.
And, unless we're going to coin a new user-level terminology, it's better to 
leave "main-target" class name --- so that class has the same name as 
user-level entity.

> - Rename "basic-target" -> something like "target-spec". Again, not
> a target but a formula.

-1 on "-spec". The name "basic-target" is indeed not excellent. Maybe 
"main-target-alternative" would better represent the role?

> - Eliminate or rename "typed-target". Pending David's review of the
> "generate" process.

I don't understand this one. Instances of 'typed-target' use generators, and 
instances of other classes do not. What's the point?

> - Rename "virtual-target" -> "target". These are the real objects
> that are associated directly with targets.

Are they? If you create virtual-target with certain name it is just hanging in 
the air, untill you decide you really want to generate it.

> o Project-root/Jamfile
>
> The requirement of having a "project-root.jam" file in addition to a
> "Jamfile" for every project means that each simple single-Jamfile
> project requires an additional file.
>
> In fact, the Jamfile is the unnecessary one of the two, which leads
> us to propose that the file performing the function of the current
> "project-root.jam" be called "Jamfile", allowing simple projects to
> exist with a single Jamfile. The current function of "Jamfile" is
> really as a sub-Jamfile and should be called something else, but
> we're not sure quite how to rename.
>
> project-root.jam -> Jamfile, Jamroot ??
> Jamfile -> SubJamfile ??

Hmm..... on Jabber I told David that I don't like this idea. If you have the 
same code used as standalone project and as part of other project, then the
same file must be called "Jamfile" in the first case and "SubJamfile" in 
another case. That's not convenient and I'd say even strange. I'd expect to 
do something for standalone project, e.g. adding empty project-root.jam, but 
changing the name of file looks like build system quirk. And the fact that 
you can rename them means they are not different. 
I'm -1 on this one.

Also, as Rene already mentioned, you need project-root.jam only at the top of 
your main project. Is this much of a problem?

- Volodya --Boundary-00=_jHIw+WHKpqz+m1Z Content-Type: text/plain;
charset="koi8-r";
name="packages.txt"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename="packages.txt"

+---------+
| Startup | +-------+---------------+ +-------+
+-------+-+ | | | | |
| | | +-------+ | |
V | V | V | V
+---------------+--+ +---------+--+ +--------------+--+ +---------+
| Abstract targets | | Generators | | Virtual targets | | Toolset |
+------------------+ +------------+ +-----------------+ +---------+
^ ^ ^ ^
| | | |
+------------------+--------+--------+----------------+
|
+---------------------+ |
| Properties/features | +-------+
+---------------------+ | Tools |
+-------+
+----------+
| Utilties |
+----------+

Note: depencies on "properties/features" and "Utilities" components are not
shown --- almost every other component depends on them.

------------------------------------------------------------------------------
STARTUP

boost-build
bootstrap
build-system

------------------------------------------------------------------------------
ABSTRACT TARGETS

project
project-root
targets

------------------------------------------------------------------------------
GENERATORS

generators

------------------------------------------------------------------------------
VIRTUAL TARGETS

virtual-target 
scanner

------------------------------------------------------------------------------
TOOLSET

toolset
type

------------------------------------------------------------------------------
PROPERTIES/FEATURES

feature
property
property-set
build-request

------------------------------------------------------------------------------
TOOLS

builtin
common

LOW-LEVEL TOOLS

alias
make
prebuilt
stage
testing

HIGH-LEVEL TOOLS

gcc
msvc
borland
kylix 
bison
lex
stlport
qt

------------------------------------------------------------------------------
UTILITIES

class
modules
assert
errors
sequence
numbers 
regex
set
string 
container
doc
os
path
print
utility
version 
 --Boundary-00=_jHIw+WHKpqz+m1Z--