307494 - [client] post-mozilla-rel.pl - PreBuild() should be able to delete objdir instead of srcdir

Status: RESOLVED FIXED

(Webtools Graveyard :: Tinderbox, defect)

Description

[Robert Kaiser]

http://lxr.mozilla.org/mozilla/source/tools/tinderbox/post-mozilla-rel.pl#879
calls "rm -rf mozilla" when wanting to clobber the build. This works well enough
for srcdir builds, but for objdir builds, we delete the source but leave the
generated objects, which is really wrong.
We should kill the objdir there, IMO, which would also enable a box using objdir
to keep patches in its tree that are not getting wiped out on every release cycle.

I think I have a working patch for that in my trip tinderbox now, but I have to
test that before attaching it :)

Comment 1

[Robert Kaiser]

Attachment: patch: kill objdir instead of "mozilla"

This patch seems to work correctly, as this is what trip just said in its log:

starting nightly release build
rm -rf mozilla/../build/

This way, we really clobber on objdir release boxen instead of just repulling
the source.

Comment 2

[Chase Phillips]

It's good we're cleaning up old objdirs that are siblings (not children) of the
main source directory.  We happen to rely on removing the source directory,
however, as a part of our nightly clean-up process.  It removes stale CVS files
and any strange build config issues that are introduced in the previous day's cycle.

Care to come up with a patch that by default removes the source and object
directories based on options in tinder-config.pl (default to yes) and could be
overridden so that either or both of the source/object directories don't get
removed?

Comment 3

[Robert Kaiser]

OK, I'll try to come up with such a patch, but I don't know how soon I'll find
time for it...

Comment 4

[Robert Kaiser]

Attachment: patch v2: make it configurable

With this patch, we should only clean up the directories we have configured. By
default, it's set to only delete the objdir; when no objdir is set, we clean up
the srcdir when either of the two prefs is set to 1.
As building with no objdir is the default config, we should do the same as
before in default config.

Comment 5

[Chase Phillips]

Comment on attachment 195913 [details] [diff] [review]
patch v2: make it configurable

This doesn't clean both objdir and srcdir by default.  r+ when you set
clean_srcdir to '1' in tinder-config.pl and tinder-defaults.pl.

Comment 6

[Chase Phillips]

This bug's summary is misleading.  PreBuild() shouldn't delete objdir instead of
srcdir.  PreBuild() should be able to do one, the other, both, or neither.  The
default behaviour shouldn't change (clean all parts of the build process).

Maybe change the summary to 'PreBuild() should be able to delete objdir instead
of srcdir' to avoid any misunderstanding.

Comment 7

[Robert Kaiser]

Attachment: patch v3: default to deleting srcdir as well

Chase:
This should delete both objdir and srcdir by default, even on objdir builds
(srcdir builds are still removing the single existing dir only once).

BTW, I don't know tinderbox tree rules too well, is your review enough to check
it in?

Comment 8

[Chase Phillips]

Comment on attachment 196033 [details] [diff] [review]
patch v3: default to deleting srcdir as well

Thanks Robert!

Comment 9

[Chase Phillips]

(In reply to comment #7)
> BTW, I don't know tinderbox tree rules too well, is your review enough to check
> it in?

Yes.  coop and bsmedberg can provide valuable feedback, as well.

Comment 10

[Robert Kaiser]

checked in (had to unbitrot it before because the deletion of l10n dir was added
in yesterday).

Comment 11

[Robert Kaiser]

Unfortunately I have to reopen this bug, as my new code had two flaws that add
up in a way so that it works as before (which also means deletion of srcdir
still works and is done in any case).

Both problems weren't seen in reviews as well and somehow I spotted them in
testing only now, though they must have been there from the beginning of making
it configurable.

bug #1: the lines that ensure for existence of the settings check for the wrong
settings, so they always set both to 1.

bug #2: when srcdir is deleted prior to objdir, the relative URL pointing to the
objdir fails (as it starts with the srcdir location).

I'm attaching a fix in a minute.

Comment 12

[Robert Kaiser]

Attachment: fix up flaws in checked in code

Sorry for the problems, here's the fix:

bug #1 described above is fixed by using the correct setting names.
bug #2 is fixed by deleting the obdir before the srcdir (in case both are
deleted)

Comment 13

[Chase Phillips]

Comment on attachment 199162 [details] [diff] [review]
fix up flaws in checked in code

I should've caught it, too.

>     if ($Settings::clean_objdir) {
>       my $objdir = 'mozilla/'.$Settings::ObjDir;
>       TinderUtils::run_shell_command "rm -rf $objdir";
>     }

Can you add an additional test in the if here to ensure that $Settings::ObjDir
is set to something and that the directory 'mozilla/'.$Settings::ObjDir is
valid before proceeding in that block?

Comment 14

[Robert Kaiser]

Attachment: fixup version 2: check for existance of the dirs before removing them

(In reply to comment #13)
> Can you add an additional test in the if here to ensure that
$Settings::ObjDir
> is set to something and that the directory 'mozilla/'.$Settings::ObjDir is
> valid before proceeding in that block?

The test directly above the deletion of objdir already checks for empty
$Settings::ObjDir and sets clean_objdir to 0 in that case, so the first part of
your request is already done.
The second part is fixed by this new patch. I've added an additional check for
both objdir and srcdir, so that we do the same for all directories, even if
srcdir must have been killed manually to be inexistent.

Comment 15

[Robert Kaiser]

fixup checked in, this should really be fixed now :)