bug#68083: 30.0.50; Intermittent build failure with native compilation

[Aaron Jensen <aaronjensen@gmail.com>]

On Sat, Dec 30, 2023 at 12:43 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Aaron Jensen <aaronjensen@gmail.com>
> > Date: Sat, 30 Dec 2023 10:29:16 -0500
> > Cc: acorallo@gnu.org, 68083@debbugs.gnu.org, jschmidt4gnu@vodafonemail.de
> >
> > On Sat, Dec 30, 2023 at 1:47 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > >
> > > Are you sure?  We do that on Windows because Windows doesn't allow us
> > > to delete a file that is open by another program.  That shouldn't
> > > happen on Posix systems, so I think what you see here is due to a race
> > > between checking whether a file exists and renaming it, which is a
> > > different problem.
> > >
> > > However, feel free to try the same trick we use on Windows and see
> > > whether it helps.
> >
> > This fixes it for me:
> >
> > diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
> > index 3b2fd25e61c..80088f935a4 100644
> > --- a/lisp/emacs-lisp/comp.el
> > +++ b/lisp/emacs-lisp/comp.el
> > @@ -3341,7 +3341,11 @@ comp-delete-or-replace-file
> >          ;; is currently loaded.
> >          (t (delete-file oldfile)
> >             (when newfile
> > -             (rename-file newfile oldfile)))))
> > +             (condition-case _
> > +                 (rename-file newfile oldfile)
> > +               (file-already-exists
> > +                (delete-file newfile)
> > +                t))))))
>
> What happens if, instead of wrapping rename-file in condition-case,
> you change that to say
>
>   (t (if newfile
>          (rename-file newfile oldfile)
>        (delete-file oldfile))

I don't really understand this change. The previous version of the
code wraps the rename in a (when newfile) and deletes the oldfile
right before that. The deletion would always be necessary if the
oldfile exists unless OK-IF-ALREADY-EXISTS is specified in
rename-file.

I tried it anyway for diligence, and I get the exact same behavior
(the rename fails because the file exists).

> > I imagine that this is worth doing just to make this operation
> > parallel-safe, but I wonder why macroexpand is the only instance of
> > this happening. I don't know if macroexpand should still be in
> > native-comp-never-optimize-functions or not (i.e., is there another
> > reason it was there other than to avoid this crash?)
>
> rename-file is supposed to be an atomic operation on Posix
> filesystems, so I don't quite understand why you see what you see, and
> I'm hesitant to sweep under the carpet a problem we don't understand.

Indeed, but we aren't specifying OK-IF-ALREADY-EXISTS, so if another
process does the same compile at the same time (and therefore the same
rename), they will conflict. We could specify OK-IF-ALREADY-EXISTS
instead and that works as well:

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index 3b2fd25e61c..d56c69d9470 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -3341,7 +3341,7 @@ comp-delete-or-replace-file
         ;; is currently loaded.
         (t (delete-file oldfile)
            (when newfile
-             (rename-file newfile oldfile)))))
+             (rename-file newfile oldfile t)))))

 (defun comp--native-compile (function-or-file &optional with-late-load output)
   "Compile FUNCTION-OR-FILE into native code.

Aaron