KDE/kdevplatform

[Andreas Pakulat]

On 09.07.09 16:54:36, Aleix Pol wrote:
> On Wed, Jul 8, 2009 at 9:32 PM, Andreas Pakulat <apaku at gmx.de> wrote:
> > On 08.07.09 18:17:02, Aleix Pol Gonzalez wrote:
> > > SVN commit 993467 by apol:
> > >
> > > A little bit of refactoring and reorganization on the buildsets.
> > > Simplifies the buildset creation a lot.
> > >
> > > If it does not fix the bug, we will have to create some kind of
> > "restructuring" state for the project or just add the new tree to the
> > project when it's ready.
> >
> > Ok, took the time now to do a complete review. I still don't like this
> > commit, it mixes too many things within the same commit.
> >
> > > ---
> > trunk/KDE/kdevplatform/plugins/projectmanagerview/builditembuilderjob.cpp
> > #993466:993467
> > > @@ -29,7 +29,9 @@
> > >  {
> > >      foreach( const BuildItem &item, items )
> > >      {
> > > -        addItem( t, item.findItem() );
> > > +        KDevelop::ProjectBaseItem *it=item.findItem();
> > > +        if(it)
> > > +            addItem( t, it );
> > >      }
> >
> > I don't think this is the right fix for the problem. If an item from the
> > buildset is currently being reloaded, we shouldn't build the buildset at
> > all, because we cannot know what changes are being done to the cmake files
> > (that are the cause of the reload). We should just disable building as long
> > as the reloading takes if a buildset is active.
> 
> agree. but we still don't want it to crash until it's done.

Well, with the crash we were at least reminded of the problem, now its easy
to forget about it. Besides I actually did plan to look into the crash
today/tomorrow ever since the last additional info was posted :)
 
> > >  BuildItem::BuildItem( const BuildItem& rhs )
> > >  {
> > > -    m_itemName = rhs.itemName();
> > > -    m_projectName = rhs.projectName();
> >
> > This is not going to work in the long term. We have open reports from
> > people that would like to put in arbitrary items into the buildset that are
> > not in the project tree. Which is actually needed for proper
> > custom-makefile support (unless someone writes a makefile parser that
> > produces meaningful project tree with all targets).
> 
> The project is not needed, it's in the path.

Ok, so I'd get

foobar | myproject

In the buildset. That should work.
 
> > We need at least an "item name" to indicate the custom target.
> >
> We can have it in the path some way. Or when we need it, we add it. I don't
> think it's a good idea to have it specified before having it.

I was about to say that allowing the user to add arbitrary items to the
project tree (which would be needed to have this available as path) opens
up a can of worms. While I still think that way, it also is a nice way of
integrating custom, unkown targets and have the custom-targets support
instantly not only in buildsets but also as possible dependency for
execution.

So, ok, you convinced me :)
 
Andreas

-- 
Try the Moo Shu Pork.  It is especially good today.