[llvm-commits] [PATCH] Fix for DragonEgg build launcher scripts and switch to ScriptedBuilder

[Galina Kistanova]

Hello Duncan,

> I don't understand what you mean by an infinite loop of links.
> What might happen is that the link gets placed inside the
> pre-existing directory.

Right. And this is the exact problem.
Here is what happens on Linux centos 2.6.18 if the script gets run
twice without removing the links in between runs.
Let's assume we have an empty "source code" directory.

$ ls -l
drwxr-xr-x 2 buildslave staff 4096 May 22 16:51 dragonegg.src

First time you create a symbolic link, it works like this:

$ ln -sf dragonegg.src ./dragonegg
$ ls -l
lrwxrwxrwx 1 buildslave staff   13 May 22 16:58 dragonegg -> dragonegg.src
drwxr-xr-x 2 buildslave staff 4096 May 22 16:51 dragonegg.src
$ ls -l ./dragonegg.src
$

Which looks right.
But the next time the same link commands get run, things go wrong:

$ ln -sf dragonegg.src ./dragonegg
$ ls -l
lrwxrwxrwx 1 buildslave staff   13 May 22 16:58 dragonegg -> dragonegg.src
drwxr-xr-x 2 buildslave staff 4096 May 22 17:04 dragonegg.src
$ ls -l ./dragonegg.src
lrwxrwxrwx 1 buildslave staff 13 May 22 17:04 dragonegg.src -> dragonegg.src

As you can see, there is a symbolic link gets placed inside the
pre-existing directory which reference itself or a parent directory.
Because the build involves recurrent iteration through nested
directories, it will eventually get an error by de-referencing
dragonegg.src/dragonegg.src/dragonegg.src symbolic link.

Am I missing something?

> Moving the "cd" up seems pointless.

This is not too important, really. It just explicitly specifies that
all relative paths we may get as a parameter will be relative to the
build directory and not the directory the script was run from. This
small change makes things easier on the buildbot side. I'll skip it if
you have a strong preference of having it that way.

> exists -> exist

Thanks for catching this!

> Also, the comment is wrong, since if the target link exists you do
> still create a new link.

Not really. Except for -h and -L, all FILE-related tests de-reference
symbolic links. Like this:

$ ls -l
lrwxrwxrwx 1 13 May 22 16:58 dragonegg -> dragonegg.src
drwxr-xr-x 2 4096 May 22 17:04 dragonegg.src

$ if [ ! -d ./dragonegg.src ] ; then echo 'Create link' ; fi
$ if [ ! -d ./dragonegg ] ; then echo 'Create link' ; fi
$ if [ ! -d ./dragonegg.new ] ; then echo 'Create link' ; fi
Create link

So, since the symbolic links actually get de-referenced by -d test,
everything should work well.
Am I missing something here?

> If the source already exists as a directory, presumably something funny
> is going on.
...
> In fact wouldn't it be better to bail out if the target exists
> and is not a symbol link?

I have thought of it. But ended up with the idea that since the
symbolic link is just a way to enforce some source code directory
naming and place convention, we shouldn't punish a user if he/she has
provided it already in the right place. We have to check this case
anyway, and it is equally simple to handle it correctly or bail out
with an error, so my choice was to handle. But I can easily make it to
bail out, if you have a strong preference for that.

What do you think?

Thanks

Galina


On Tue, May 25, 2010 at 4:39 AM, Duncan Sands <baldrick at free.fr> wrote:
> Hi Galina,
>
>> patch07.diff fixes a possible infinite loop linked with ln commands,
>> and changes directory before the ln command so it could dial well with
>> the relative to the build root pathes.
>>
>> Both buildbot_self_strap and buildbot_self_strap-32 scripts could make
>> infinite loop of links on Linux if the build directories remain
>> between builds. This could happen because the second time "form 3 of
>> ln command" will be used.
>
> thanks for working on this.  I don't understand what you mean by an infinite
> loop of links.  What might happen is that the link gets placed inside the
> pre-existing directory.
>
>> -ln -sf $LLVM_SOURCE $BUILD_DIR/llvm
>> -ln -sf $DRAGONEGG_SOURCE $BUILD_DIR/dragonegg
>> +# Change the current directory to the build root.
>> +cd $BUILD_DIR
>
> Moving the "cd" up seems pointless.
>
>> +# Create links only if target directory or link does not exists.
>
> exists -> exist
> Also, the comment is wrong, since if the target link exists you do
> still create a new link.
>
>> +if [ ! -d $BUILD_DIR/llvm ] ; then
>> +   ln -sf $LLVM_SOURCE $BUILD_DIR/llvm
>> +fi
>> +if [ ! -d $BUILD_DIR/dragonegg ] ; then
>> +   ln -sf $DRAGONEGG_SOURCE $BUILD_DIR/dragonegg
>> +fi
>
> If the source already exists as a directory, presumably something funny
> is going on.  Wouldn't it be better to bail out with an error in this
> case?  In fact wouldn't it be better to bail out if the target exists
> and is not a symbol link?
>
> Ciao,
>
> Duncan.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>