Fix Hurd build with read-only source directory

Message ID alpine.DEB.2.21.1811271811500.11571@digraph.polyomino.org.uk
State Committed
Headers

Commit Message

Joseph Myers Nov. 27, 2018, 6:12 p.m. UTC
  The logic for generating sysdeps/mach/hurd/bits/errno.h involves a
stamp file and $(move-if-change).

The temporary file (generated unconditionally) is generated in the
source directory.  This means that even if
sysdeps/mach/hurd/bits/errno.h is up to date, and has an up to date
timestamp, the build will fail if the source directory is read-only.
Even with a writable source directory, multiple concurrent builds for
i686-gnu with the same source directory could race to access the
temporary file (which always has the same name).

This patch uses the build directory for the temporary file instead to
avoid those problems.  (In the case where the file is out of date and
the temporary file does need to be moved to the source directory, if
there are multiple concurrent builds for i686-gnu with the same source
directory, and the source and build directories are on different
filesystems, it's possible there might still be races replacing the
file in the source directory, depending on exactly how mv handles such
cross-filesystem moves.  This is certainly no worse than the present
situation, where such a case would have races regardless of whether
the file is out of date or whether different filesystems are in use.)

Tested with a build-many-glibcs.py build for i686-gnu.

2018-11-27  Joseph Myers  <joseph@codesourcery.com>

	* sysdeps/mach/hurd/Makefile ($(common-objpfx)stamp-errnos): Use
	$(hurd-objpfx)bits/errno.h-tmp, not $(hurd)/bits/errno.h-tmp.
  

Comments

Samuel Thibault Nov. 27, 2018, 6:32 p.m. UTC | #1
Joseph Myers, le mar. 27 nov. 2018 18:12:28 +0000, a ecrit:
> The logic for generating sysdeps/mach/hurd/bits/errno.h involves a
> stamp file and $(move-if-change).
> 
> The temporary file (generated unconditionally) is generated in the
> source directory.  This means that even if
> sysdeps/mach/hurd/bits/errno.h is up to date, and has an up to date
> timestamp, the build will fail if the source directory is read-only.
> Even with a writable source directory, multiple concurrent builds for
> i686-gnu with the same source directory could race to access the
> temporary file (which always has the same name).
> 
> This patch uses the build directory for the temporary file instead to
> avoid those problems.

Right :)

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> (In the case where the file is out of date and
> the temporary file does need to be moved to the source directory, if
> there are multiple concurrent builds for i686-gnu with the same source
> directory, and the source and build directories are on different
> filesystems, it's possible there might still be races replacing the
> file in the source directory, depending on exactly how mv handles such
> cross-filesystem moves.  This is certainly no worse than the present
> situation, where such a case would have races regardless of whether
> the file is out of date or whether different filesystems are in use.)

Indeed. I guess we don't want to care about concurrent builds on
different systems?  A way to make it safer would be to move the file in
two steps: in scripts/move-if-change, after testing for difference, move
to "$2.$$", thus in the same directory but with a name which shouldn't
be getting conflicts, then move from "$2.$$" to "$2", which will have
filesystem-provided atomicity.

In practice, I wouldn't expect people to make parallel builds in a case
where the file needs to be updated.

Samuel
  
Joseph Myers Nov. 27, 2018, 10:06 p.m. UTC | #2
On Tue, 27 Nov 2018, Samuel Thibault wrote:

> Indeed. I guess we don't want to care about concurrent builds on
> different systems?  A way to make it safer would be to move the file in
> two steps: in scripts/move-if-change, after testing for difference, move
> to "$2.$$", thus in the same directory but with a name which shouldn't
> be getting conflicts, then move from "$2.$$" to "$2", which will have
> filesystem-provided atomicity.

Note that move-if-change comes from gnulib, so any such improvements would 
need to go there first.
  

Patch

diff --git a/sysdeps/mach/hurd/Makefile b/sysdeps/mach/hurd/Makefile
index e15341ca8d..69f12d4efb 100644
--- a/sysdeps/mach/hurd/Makefile
+++ b/sysdeps/mach/hurd/Makefile
@@ -91,10 +91,11 @@  $(common-objpfx)errnos.d: $(mach-errnos-deps)
 $(hurd)/bits/errno.h: $(common-objpfx)stamp-errnos ;
 $(common-objpfx)stamp-errnos: $(hurd)/errnos.awk $(errno.texinfo) \
 			      $(mach-errnos-deps) $(common-objpfx)errnos.d
-	$(AWK) -f $^ > $(hurd)/bits/errno.h-tmp
+	mkdir -p $(hurd-objpfx)bits
+	$(AWK) -f $^ > $(hurd-objpfx)bits/errno.h-tmp
 # Make it unwritable so noone will edit it by mistake.
-	-chmod a-w $(hurd)/bits/errno.h-tmp
-	$(move-if-change) $(hurd)/bits/errno.h-tmp $(hurd)/bits/errno.h
+	-chmod a-w $(hurd-objpfx)bits/errno.h-tmp
+	$(move-if-change) $(hurd-objpfx)bits/errno.h-tmp $(hurd)/bits/errno.h
 	touch $@
 
 common-generated += errnos.d stamp-errnos