208714 - checksetup.pl dies when it cannot remove $datadir/template

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Jason Pyeron]

User-Agent:       Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0; .NET CLR 1.0.3705)
Build Identifier: 

checksetup.pl was able to remove all of the files.
but since one was open (hence a directory handle was still valid)
it could not unlink the parent directory.

if it whined non-fatal then tried to recompile
it would of found that the dirs were completely empty and could proceed anyways.


suggested solution would be to verify that there are no NON-DIR entries under 
the data/template as a check of complete deletion.

if then a subsequent write error occurs then whine (fatal) about that too.

OTHERWISE you have to track down which process is open, and close it. (This was 
inconvenient since it was my text editor which had much work I was not ready to 
save/loose the undo buffer)


Reproducible: Always

Steps to Reproduce:

Comment 1

[Bradley Baetz (:bbaetz)]

> (This was  inconvenient since it was my text editor which had much work I was
> not ready to save/loose the undo buffer)

Why are you editing stuff in data/template? If you want to edit the template,
edit stuff in template/*.

We an't just get write errors, since one of the reasons we delete everything is
to make sure that files which have been deleted from template/ don't end up in
data/template.

Comment 2

[Jason Pyeron]

I was READING, not editing. besides that is not relevant.

besides what I said was to confirm that all NON-DIR files are gone. 

that means only DIRS maybe left, and STILL whine about it, but dont be fatal

Comment 3

[Jason Pyeron]

Attachment: recover on fail to delete data/template skeleton

Removing existing compiled templates ...
Can't remove directory data/template/en/default/bug/process: Permission denied
at C:\usr\jpyeron\proj\JBUGZI~1\Bugzilla\tip\CHECKS~1.PL line 929
Can't remove directory data/template/en/default/bug: Directory not empty at
C:\usr\jpyeron\proj\JBUGZI~1\Bugzilla\tip\CHECKS~1.PL line 929
Can't remove directory data/template/en/default: Directory not empty at
C:\usr\jpyeron\proj\JBUGZI~1\Bugzilla\tip\CHECKS~1.PL line 929
Can't remove directory data/template/en: Directory not empty at
C:\usr\jpyeron\proj\JBUGZI~1\Bugzilla\tip\CHECKS~1.PL line 929
Can't remove directory data/template: Directory not empty at
C:\usr\jpyeron\proj\JBUGZI~1\Bugzilla\tip\CHECKS~1.PL line 929


The data/template directory could not be removed. Please
remove it manually and rerun checksetup.pl.

Will attempt to recover...the data/template and sub-directories are empty of
non-directories, proceding anyways...

Precompiling templates ...

Comment 4

[Bradley Baetz (:bbaetz)]

Comment on attachment 125205 [details] [diff] [review]
recover on fail to delete data/template skeleton

and on windows that locks the directory, but still allows the file to be
deleted? I thought open files couldn't be deleted at all.

+ #my DIR;    # how do I do this?

Under perl>=5.6, |my $dir| will work, and opendir will make that into a
filehandle.

However, the best thing to do would be to use File::Find, and have the wanted
function display the error message and exit if it runs into something which
isn't a dir, using something like (untested):

sub {
  if (! -d $File::Find::name) {
    print "....";
    exit 1;
  }
}

Your version will loop forever if theres a symlink loop. It'll also go outside
the directory tree if there is a symlink. File::Find avoids all of that.

The other issue is what if theres somehow a directory with the name of one of
the files we want? If it can't be deleted, this change will let it go.

I still think that this should be an error, and you should have to close your
editor, but I'm not sure what others think.

Comment 5

[Jason Pyeron]

Attachment: recover on fail to delete data/template skeleton

re: and on windows that locks the directory, but still allows the file to be
deleted? 

Yes, this is the case, the file handle will still be valid for any applications
which have it, but no new apps will be able to obtain it. It is just unlinked
from the filesystem, but not paged out of memory.


re: Your version will loop forever if theres a symlink loop. It'll also go
outside
the directory tree if there is a symlink. File::Find avoids all of that.

That is not true unless -d evals true on symlinks. Look closely at the code

ignore special dirs . & ..
if you encounter a NONdir entry abort and return 0;
otherwise it is a dir (-d) recurse

if no NONdirs found return success


re: The other issue is what if theres somehow a directory with the name of one
of the files we want? If it can't be deleted, this change will let it go.

Yes, this is avery good point, I wanted to do ./checksetup.pl --force
but, that would conflict with the establised usage and was going to get messy.
checksetup will still fail when the template is tired to be compiled (right?)
if the file exists.


re: I still think that this should be an error, and you should have to close
your editor, but I'm not sure what others think. 

But if I have 19 important OTHER files open, that too much to ask (i know it
should release the dir handle, but it is 11 year old software).

Comment 6

[Dave Miller [:justdave]]

I tend to agree with Brad on this...  if you want a 100% certain clean compile,
you're going to want all the directories gone, too.

Comment 7

[Jason Pyeron]

Dave, Brad,

Here is your dredded a dir exists whose name is the name of a template which 
will need to exist.

C:\usr\jpyeron\proj\jBugzilla\Bugzilla\tip>checksetup.pl
<...snip...>
Removing existing compiled templates ...
Can't remove directory data/template/en/default/bug/process: Permission denied 
at C:\usr\jpyeron\proj\JBUGZI~1\Bugzilla\tip\CHECKS~1.PL line 937
Can't remove directory data/template/en/default/bug: Directory not empty at 
C:\usr\jpyeron\proj\JBUGZI~1\Bugzilla\tip\CHECKS~1.PL line 937
Can't remove directory data/template/en/default: Directory not empty at 
C:\usr\jpyeron\proj\JBUGZI~1\Bugzilla\tip\CHECKS~1.PL line 937
Can't remove directory data/template/en: Directory not empty at 
C:\usr\jpyeron\proj\JBUGZI~1\Bugzilla\tip\CHECKS~1.PL line 937
Can't remove directory data/template: Directory not empty at 
C:\usr\jpyeron\proj\JBUGZI~1\Bugzilla\tip\CHECKS~1.PL line 937


The data/template directory could not be removed. Please
remove it manually and rerun checksetup.pl.


C:\usr\jpyeron\proj\jBugzilla\Bugzilla\tip>

and then ...

C:\usr\jpyeron\proj\jBugzilla\Bugzilla\tip>checksetup.pl --force
<...snip...>
Removing existing compiled templates ...
Can't remove directory data/template/en/default/bug/process: Permission denied 
at C:\usr\jpyeron\proj\JBUGZI~1\Bugzilla\tip\CHECKS~1.PL line 937
Can't remove directory data/template/en/default/bug: Directory not empty at 
C:\usr\jpyeron\proj\JBUGZI~1\Bugzilla\tip\CHECKS~1.PL line 937
Can't remove directory data/template/en/default: Directory not empty at 
C:\usr\jpyeron\proj\JBUGZI~1\Bugzilla\tip\CHECKS~1.PL line 937
Can't remove directory data/template/en: Directory not empty at 
C:\usr\jpyeron\proj\JBUGZI~1\Bugzilla\tip\CHECKS~1.PL line 937
Can't remove directory data/template: Directory not empty at 
C:\usr\jpyeron\proj\JBUGZI~1\Bugzilla\tip\CHECKS~1.PL line 937


The data/template directory could not be removed.
Will attempt to recover or exit...

The data/template and sub-directories are NOW empty
of non-directories, proceding anyways...

Precompiling templates ...
Could not compile bug/process/midair.html.tmpl: cache failed to write 
midair.html.tmpl: Permission denied

C:\usr\jpyeron\proj\jBugzilla\Bugzilla\tip>

Comment 8

[Jason Pyeron]

as you can see from comment 7

the checksetup still fails with a usable message when this condition exists.

I dont see how this would hinder operation if the patch is included.
But more over, without the patch, operation is hindered. This also places some 
headway for other "forcing" operations in checksetup

Comment 9

[Zach Lipton [:zach]]

standard triage jazz

Comment 10

[Jason Pyeron]

re comment 9:

Minor
 minor loss of function, or other problem where easy workaround is present  

Trivial
 cosmetic problem like misspelled words or misaligned text  

Could you please justify how this is "Trivial"? This is a minor problem where 
checksetup assumes that it cannot complete its task when it might be able to. 
The easy workaround is to free the OS resources which are in conflict.

Comment 11

[Jason Pyeron]

or the patch attachment 125209 [details] [diff] [review].

Comment 12

[Jason Pyeron]

Attachment: recover on fail to delete data/template skeleton

Comment 13

[Bradley Baetz (:bbaetz)]

Comment on attachment 127893 [details] [diff] [review]
recover on fail to delete data/template skeleton 

You need to use file::Find, not opendir/readdir.

Comment 14

[Jason Pyeron]

What is your rationale?

The File::Find architecture seems awkward and unclear. Further it is unclear if 
it can short-circuit.

Comment 15

[Jason Pyeron]

it also increases dependencies:

use File::Find;

Comment 16

[Jason Pyeron]

Attachment: recover on fail to delete data/template skeleton

please review nofiles2 vs nofiles

they both work.

I am still looking for a rationale to use File::Find (which uses
opendir/readdir) vs directly using opendir/readdir.

Can specific advantages / disadvantages be listed?

Comment 17

[Jason Pyeron]

Comment on attachment 128076 [details] [diff] [review]
recover on fail to delete data/template skeleton 

this patch has both code sets included. Please justify why File::Find is more
preferable.

nofiles2 (File::Find) is being used in this patch.

Comment 18

[Jason Pyeron]

Attachment: recover on fail to delete data/template skeleton

Refixed the symlink issue. Oops.

Comment 19

[Jason Pyeron]

Attachment: recover on fail to delete data/template skeleton

Our perl guru is back, Yea!!!!

So it now short circuits,
handles mac file system
uses File::Find
no globals
no extra subs


here is the jist:
 my $res=1;
 eval{find(sub{if(!-d){$res=0;die;}},$cdir);};
 return $res;

only caveat may be if inside the eval block an exception is thrown which is not
handled, may lead to false +?

we looking into this.

Comment 20

[Jason Pyeron]

Attachment: recover on fail to delete data/template skeleton

added the obscure/unforeseen error checking

Comment 21

[Zach Lipton [:zach]]

Correct me if I'm wrong, but my understanding of this patch is to do the following:

If data/templates was not removed when we attempted to remove it, see if there
are only directories in data/templates.

If there are only directories in data/templates and --force was passed, don't
throw a fatal error and move on.

If --force was passed but data/templates contains non-directories, die anyway.

If --force was not passed, die.

It appears that the use of the --force option is not documented. Am I missing
something? Also, should the option be named --force-delete-templates (or
something like that) to define what exactly is being forced?

Am I correct with this logic?

Comment 22

[Jason Pyeron]

re: Am I correct with this logic?

yes

re: It appears that the use of the --force option is not documented. Am I 
missing something? Also, should the option be named --force-delete-templates (or
something like that) to define what exactly is being forced?

--force has never existed before.
I decided to add a general --force for this since it adds the "logic" for other 
issues, not discussed here.

why not --force-delete....?
it would add much to the checksetup file. Since the developers have frowned on 
mutations like this, even non-standard operations like not deleting template 
dirs, it seemed OK to generalize all FORCE operations. But this is for a 
different BUG/RFE

Comment 23

[Bradley Baetz (:bbaetz)]

Comment on attachment 128115 [details] [diff] [review]
recover on fail to delete data/template skeleton

Add a bit more spacing/newlines arround the eval line, and r=bbaetz.

Comment 24

[Jason Pyeron]

something like?:

eval
{
 find
 (
  sub
  {
   if(!-d)
   {
    $res=0;die;
   }
  }
  ,$cdir
 );
};

Comment 25

[Zach Lipton [:zach]]

Jason, I would think something more like: 

eval {
  find (
    sub {if(!-d){
            $res=0;die;}}
  ,$cdir);
};

(that's pretty ugly too, but what you pasted before is a lot of lines). *Sigh*
there's no pretty way to format find() calls. 

I'd be happy to check this in for you once it has approval.

Also, how do we plan to address the documentation of the --force switch? Perhaps
it should tell the user they can use --force when you say that data/templates
could not be removed? 

Zach

Comment 26

[Zach Lipton [:zach]]

Comment on attachment 128115 [details] [diff] [review]
recover on fail to delete data/template skeleton

>+my $force;
>+if ($ARGV[0] && $ARGV[0] eq "--force") {
>+    # [e-mail] Bug #208714 
>+    shift @ARGV;
>+    $force = 1;
>+}

What if $ARGV[0] is some other switch (--silent perhaps?) and --force is in
$ARGV[1]? I still don't like the idea of using just --force? Force what to
happen?

Comment 27

[Jason Pyeron]

Attachment: recover on fail to delete data/template skeleton

Added some formatting to ?everyone's? liking.
Added some comments to the --force section
 They included usage info, warnings, etc.

document??? where should --force be documented. I looked for the silent docs
and not found.

C:\...\Bugzilla\tip\docs>grep -Ri "silent" *

C:\...\Bugzilla\tip\docs>grep -Ri "interactive" *

C:\...\Bugzilla\tip\docs>

Comment 28

[Jason Pyeron]

> Perhaps it should tell the user they can use --force when ...


This is a very BAD idea.

Document it in the manual, but never encourage the --force option to be used.

Think how dangerous it is to do:

rm -f

rpm -if kernel.rpm

These applications never **suggest** a force.

there is something wrong if a --force is needed.

Ex: you do not have a correct lib installed, you have files open in an editor 
still, etc.

--force says, hey program I know you think it cannot be done or handled but I 
know better, against all the your developers have said.

which is sometime the case, hence the need for --force.


Where in the manual is non-interactive operation covered?

Comment 29

[Bradley Baetz (:bbaetz)]

Comment on attachment 130753 [details] [diff] [review]
recover on fail to delete data/template skeleton

Looks fine.

Comment 30

[Jason Pyeron]

so can we get this to patch???

Comment 31

[Zach Lipton [:zach]]

Jason,

Why does --force need to be the first switch on the commandline? I think it would confuse a user 
using other switches to have to put --force first instead of second or third. 

Comment 32

[Jason Pyeron]

re comment 31 and comment 26:

a confused user should not be using a --force option ever!

but kidding aside:

I put --force first since I could use shift to keep checksetup.pl's code the 
same, that is the least amount of side effects.

until it is proposed to redo how checksetup is structured then it does not make 
sense to put a 'tar' like arg proccessing capabilites.

Comment 33

[Dave Miller [:justdave]]

Clearing approval request pending review of the current patch.  bbaetz?

Comment 34

[Dave Miller [:justdave]]

All 2.18 bugs that haven't been touched in over 60 days and aren't flagged as
blockers are getting pushed out to 2.20

Comment 35

[Jason Pyeron]

Comment on attachment 130753 [details] [diff] [review]
recover on fail to delete data/template skeleton

a pathc was approved last year when is it going to get checked in?

Comment 36

[Jason Pyeron]

Attachment: bugzilla-208714-v1.11.diff

some bit rot fixed...

Comment 37

[Jason Pyeron]

Attachment: bugzilla-208714-v1.12.diff

fixed some bit rot (oops on #11)

Comment 38

[Jason Pyeron]

docs for --force are in Bug 224417

Comment 39

[Gervase Markham [:gerv]]

Comment on attachment 160895 [details] [diff] [review]
bugzilla-208714-v1.12.diff

>+# --force must always be first

You can use "grep" to relax this requirement pretty easily.

>+    # [e-mail] Bug #208714

It's better not to annotate code in this way. Such annotations can get out of
date or confuse, and get silly when everyone does them, and anyway, that's what
CVS blame is for.

>+sub nofiles
>+{
>+ use File::Find;
>+ my ($cdir)=@_;
>+ my $res=1;
>+ eval
>+ {
>+  find( sub { if(!-d) {$res=0;die;} }
>+       ,$cdir);
>+ };
>+ $res=0 if defined $@ and $@ ne '';
>+ return $res;
>+}

This sub doesn't follow the normal spacing/indenting rules, and it definitely
needs a comment or two explaining what's going on :-)

>+            print "The $datadir/template directory could not be removed.\n";
>+            print "Will attempt to recover or exit...";
>+            exit unless (nofiles('$datadir/template'));
>+            print "\n\nThe $datadir/template and sub-directories are NOW empty\n";
>+            print "of non-directories, proceding anyways...\n\n";

Nit: "anyway" is correct English.

Gerv

Comment 40

[Dave Miller [:justdave]]

Moving documentation issue back here, now that we have a documentation flag that
is tracked separately from the rest of the bug.  This way it also doesn't show
up on the docs team's plate until the bug is resolved.

Comment 41

[Dave Miller [:justdave]]

*** Bug 224417 has been marked as a duplicate of this bug. ***

Comment 42

[Marc Schumann [:Wurblzap]]

Comment on attachment 130753 [details] [diff] [review]
recover on fail to delete data/template skeleton

Removing r? from obsolete patch

Comment 43

[Max Kanat-Alexander]

This bug has not been touched by its owner in over six months, even though it is
targeted to 2.20, for which the freeze is 10 days away. Unsetting the target
milestone, on the assumption that nobody is actually working on it or has any
plans to soon.

If you are the owner, and you plan to work on the bug, please give it a real
target milestone. If you are the owner, and you do *not* plan to work on it,
please reassign it to [e-mail] or a .bugs component owner. If you are
*anybody*, and you get this comment, and *you* plan to work on the bug, please
reassign it to yourself if you have the ability.

Comment 44

[Jason Pyeron]

when is a good time to fix this?

I dont have much time to spend on making patches right now, so if I could get 
it done a day after bug 285722 lands, can it get reviewed and checked in???


it should of happened back in july 2003, but ...

Comment 45

[Max Kanat-Alexander]

(In reply to comment #44)
> I dont have much time to spend on making patches right now, so if I could get 
> it done a day after bug 285722 lands, can it get reviewed and checked in???

  If it passes review, I'd imagine that that's always the case. :-) So that's a
"yes," but nobody's promising a r+.

Comment 46

[Jason Pyeron]

it has already passed review, before, sigh!

Comment 47

[Frédéric Buclin]

--force has not been implemented yet. So there is nothing to add to documentation.

Comment 48

[Max Kanat-Alexander]

Actually, I think the solution here is just to make $datadir/template non-deletion non-fatal.

Comment 49

[Max Kanat-Alexander]

Attachment: v13

Ha! After all these years, I finally coded up the solution to this problem. We just move the data/template directory if we can't remove it, because we're allowed to move things around on the same partition even if we can't delete them.

Granting myself review as module owner.

Comment 50

[Max Kanat-Alexander]

Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.107; previous revision: 1.106
done
Checking in template/en/default/setup/strings.txt.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/setup/strings.txt.pl,v  <--  strings.txt.pl
new revision: 1.16; previous revision: 1.15
done