132756 - fast-update doesn't handle cvs removed subdirs

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Bradley Baetz (:bbaetz)]

fast-update has two problems I've discovered. Firstly, it uses the return value
of system to check for sucess, and then does exit $status. The problem with this
is that the return value of system is not the exit code - you need $? >> 8.

As a result, the shell seems to take $status % 256, (which is 0 unless cvs core
dumped or was killed by a signal) and so make carries on normally, but the
.fast-update stamp file isn't updated.

The fix for this is to just change the system calls to return $? >> 8, and I
have this fixed in my local tree. I don't want to apply it, though, because of
another problem.

The reason I noticed this is that sometimes .fast-update stamp files aren't
updated. I looked at logs, and it seems that we fail if the following happens:

consider a directoraty structure, extentions/xmlextras/docs/soap/html (no, this
isn't a hypothetical example)

Then cvs remove extentions/xmlextras/docs/soap/html and
extentions/xmlextras/docs/soap.

Now try to update "extentions/xmlextras/docs/soap/html"

The result:

cvs [update aborted]: no such directory `extensions/xmlextras/docs/soap'

Oops. So cvs exits with code 1, and we don't update the stamp file, but make
keeps going because of the above problem.

Now, I'm not entirely certain why this happens, since I don't keep logs on my
machine, and brad-fast was pulling checkouts since Mar 9, which is past what
tbox keeps.

My guess is that the cvs update process was interrupted for some reason, or
there was a local conflict, so the stamp wasn't updated and the update was run
again.

The other option (more likely with brad-fast than my machine) is that if you
pull twice within an hour, the directories will still be considered, so the
first time everything works correctly, then subsequently it fails, and from then
on the timestamp never gets updated unless you fix it manually (which Brad did
on the machine this afternoon)

I don't know how to solve this. We could reverse the sort, but then we'd fail on
adding new directories, when trying to add the subdir before the main directory.
We could detect additions by checking to see if the directories' parent exists
locally (if not, update the base, and only update the subdir if the base still
exists), but I don't know if that works in all cases - what if a subdir really
has no files in it, only other subdirectories?.

Anyone got some better suggestions?

Comment 1

[hacker formerly known as [e-mail]]

-> bbaetz

Comment 2

[John Keiser (jkeiser)]

Attachment: Patch

OK, I tracked this problem down.  What's happening is, you (as do most people)
have update -dP in your .cvsrc.  Now when we try to update the directory, we
first check to see if it exists--if it doesn't, we update the parent.  At this
stage we do an update -dP and get the children directories.  But if -P is in
the .cvsrc, we simply don't grab the children directories, and thus the
subsequent attempt to update in the child directories is doomed.

My solution was to not grab options from .cvsrc (the -f option), which turns
off pruning; another possibility is to check if the directory was actually
created before updating and pretending we succeeded, but it seems like that
could break if the process legitimately failed to grab the subdirectories.  If
we combined that check with breaking early if there is an error, that would
probably fix that.  Finally, I think even in that case we still need to do -f
and explicitly set -P if we really want it.

My fast-update now succeeds happily through the last 950 hours of checkins
(that was how long ago it got stuck).

Comment 3

[John Keiser (jkeiser)]

However is decided to be best, I'll take this.

Comment 4

[Brad Roberts]

Either tinderbox or client.mk is adding those options.  I don't have a .cvsrc 
file.

Comment 5

[Bradley Baetz (:bbaetz)]

Comment on attachment 117733 [details] [diff] [review]
Patch

BRad: Err, you do, cause I added one the other day. I didn't think of this

Why isn't the -l sufficient, though?

Comment 6

[John Keiser (jkeiser)]

I suspect -P always prunes a directory if it's empty, period.  -l just means
don't recurse AFAICT; if there is nothing to check out in the directory it
probably prunes it.

Comment 7

[Alec Flett]

Comment on attachment 117733 [details] [diff] [review]
Patch

sr=alecf

make me wonder if we should be consolidating the calls to cvs.

Comment 8

[Alec Flett]

oh, and if we're not going to use ~/.cvsrc, can we at least add -z3 and -q?

Comment 9

[John Keiser (jkeiser)]

Wow, I forgot about this one.  Fix checked in.

Comment 10

[John Keiser (jkeiser)]

BTW, Alec, I added -z3 and -q to the options as requested.