Re: mkdir -p -- minor cleanup
Hi Paul, * Paul Eggert wrote on Mon, Apr 24, 2006 at 10:36:43PM CEST: If I understand you and Ralf correctly about race conditions, here is a proposed patch to should fix the condition on POSIX conforming hosts. On older hosts there's no hope that I can see in general, but it's not worth worrying about these ancient hosts at this point. I hope this addresses Ralf's concerns. Yes, I think so. Very clever patch, thank you! Cheers, Ralf 2006-04-24 Paul Eggert [EMAIL PROTECTED] * lib/install-sh: Close a race condition reported by Ralf Wildenhues and Stepan Kasal. There is still a race condition on hosts that predate POSIX 1003.1-1992, but we can't help this.
Re: mkdir -p -- minor cleanup
Hi Paul, * Paul Eggert wrote on Mon, Apr 24, 2006 at 10:36:43PM CEST: If I understand you and Ralf correctly about race conditions, here is a proposed patch to should fix the condition on POSIX conforming hosts. On older hosts there's no hope that I can see in general, but it's not worth worrying about these ancient hosts at this point. I hope this addresses Ralf's concerns. Yes, I think so. Very clever patch, thank you! Cheers, Ralf 2006-04-24 Paul Eggert [EMAIL PROTECTED] * lib/install-sh: Close a race condition reported by Ralf Wildenhues and Stepan Kasal. There is still a race condition on hosts that predate POSIX 1003.1-1992, but we can't help this.
Re: mkdir -p -- minor cleanup
Stepan Kasal [EMAIL PROTECTED] writes: All my complaints were considered, and we have Alexandre's approval, so I guess you could check it in. OK, thanks, I did that. If I understand you and Ralf correctly about race conditions, here is a proposed patch to should fix the condition on POSIX conforming hosts. On older hosts there's no hope that I can see in general, but it's not worth worrying about these ancient hosts at this point. I hope this addresses Ralf's concerns. 2006-04-24 Paul Eggert [EMAIL PROTECTED] * lib/install-sh: Close a race condition reported by Ralf Wildenhues and Stepan Kasal. There is still a race condition on hosts that predate POSIX 1003.1-1992, but we can't help this. --- install-sh.~1.31.~ 2006-04-24 12:58:37.0 -0700 +++ install-sh 2006-04-24 13:34:41.0 -0700 @@ -340,13 +340,16 @@ do pathcomp=$pathcomp$d if test ! -d $pathcomp; then - $mkdirprog $pathcomp + if $posix_mkdir; then + $mkdirprog -m $mkdir_mode -p -- $dstdir break + else + $mkdirprog $pathcomp obsolete_mkdir_used=true + fi # Don't fail if two instances are running concurrently. test -d $pathcomp || exit 1 fi pathcomp=$pathcomp/ done - obsolete_mkdir_used=true fi fi
Re: mkdir -p -- minor cleanup
Hello Ralf, On Sat, Apr 22, 2006 at 08:44:06PM +0200, Ralf Wildenhues wrote: It was already noted that `-m MODE' is understood by mkdir, by install_sh, and by mkinstalldirs (the latter would not be used any more, I'm just noting it here). However, we can't be sure that there is no race here, since the mode may be set strictly later than directory creation by some of those scripts. For an Autoconf macro, and maybe even for Automake, it may be worthwhile to fix this: su -c 'make install DESTDIR=/tmp/dest' should not require me to think about setting a proper umask first. a quick look shows that the current install-sh indeed contains this race condition. You are probably right that we should fix this and review current GNU mkdir before we enhace the documentation to encourage people to use $(mkdir_p) -m MODE But I don't think this is a blocker for Paul's patch. All my complaints were considered, and we have Alexandre's approval, so I guess you could check it in. Have a nice day, Stepan
Re: mkdir -p -- minor cleanup
Stepan Kasal [EMAIL PROTECTED] writes: All my complaints were considered, and we have Alexandre's approval, so I guess you could check it in. OK, thanks, I did that. If I understand you and Ralf correctly about race conditions, here is a proposed patch to should fix the condition on POSIX conforming hosts. On older hosts there's no hope that I can see in general, but it's not worth worrying about these ancient hosts at this point. I hope this addresses Ralf's concerns. 2006-04-24 Paul Eggert [EMAIL PROTECTED] * lib/install-sh: Close a race condition reported by Ralf Wildenhues and Stepan Kasal. There is still a race condition on hosts that predate POSIX 1003.1-1992, but we can't help this. --- install-sh.~1.31.~ 2006-04-24 12:58:37.0 -0700 +++ install-sh 2006-04-24 13:34:41.0 -0700 @@ -340,13 +340,16 @@ do pathcomp=$pathcomp$d if test ! -d $pathcomp; then - $mkdirprog $pathcomp + if $posix_mkdir; then + $mkdirprog -m $mkdir_mode -p -- $dstdir break + else + $mkdirprog $pathcomp obsolete_mkdir_used=true + fi # Don't fail if two instances are running concurrently. test -d $pathcomp || exit 1 fi pathcomp=$pathcomp/ done - obsolete_mkdir_used=true fi fi
Re: mkdir -p -- minor cleanup
* Stepan Kasal wrote on Wed, Apr 19, 2006 at 01:08:57PM CEST: And $(mkdir_p) cannot be used with an option anyway. It was already noted that `-m MODE' is understood by mkdir, by install_sh, and by mkinstalldirs (the latter would not be used any more, I'm just noting it here). However, we can't be sure that there is no race here, since the mode may be set strictly later than directory creation by some of those scripts. For an Autoconf macro, and maybe even for Automake, it may be worthwhile to fix this: su -c 'make install DESTDIR=/tmp/dest' should not require me to think about setting a proper umask first. Cheers, Ralf
Re: mkdir -p -- minor cleanup
Hello Paul. On Wed, Apr 19, 2006 at 04:18:04PM -0700, Paul Eggert wrote: Stepan Kasal [EMAIL PROTECTED] writes: And $(mkdir_p) cannot be used with an option anyway. But under the patch I proposed, $(mkdir_p) -m 444 would be allowed, for example. (Well, not ``for example'', -m is the only option you can use here. But this would have to be properly documented: 1) in the comment in mkdir_p, as another reason why GNU mkdir is the only one accepted, 2) in the documentation for the macro. But portable directory names cannot start with '-'. See http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap04.html#tag_04_06. I don't think we need to worry about applications that want to install files whose names begin with -. [...] Yes, this is true for Autoconf. But what about Automake, this would be a regression for it. But it could replace all uses by $(mkdir_p) --, of course. A minor advantage of not ending in ' --' is that the output of make is easier to read. Thus it seems this will apply only to packages which don't use Automake. - # $(mkinstalldirs) is defined by Automake if mkinstalldirs exists. - if test -f $ac_aux_dir/mkinstalldirs; then -mkdir_p='$(mkinstalldirs)' - else -mkdir_p='$(install_sh) -d' - fi Hmm, why remove support for mkinstalldirs here? Isn't that a separate issue? I'm not removing the support for mkinstalldirs. I just do not use it as a fallback for mkdir_p, even if it is present. (Note that every package using current Automake has to distribute install-sh, because AM_INIT_AUTOMAKE requires AC_PROG_INSTALL.) You are right, this is a separate change, but it is a prerequisite for moving AM_PROG_MKDIR_P to Autoconf, and Bruno's original patch contained it, too. I proposed it long ago: http://lists.gnu.org/archive/html/automake-patches/2005-07/msg00038.html The discussion continued this March: http://lists.gnu.org/archive/html/automake-patches/2006-03/msg00015.html A plan: --- First, the ``mkinstalldirs'' issue has to be resolved. Second, `install-sh' should be patched to understand `--', the patch is earier in this thread. Third, as you put it: [...] mkdir_p should end in ' --', or not. But it should be consistent, whichever way is chosen, so clearly a change is needed. And then we can return to the question of moving the macro to Autoconf. Have a nice day, Stepan
Re: mkdir -p -- minor cleanup
Hello, On Wed, Apr 19, 2006 at 01:02:30AM -0700, Paul Eggert wrote: [...], but now that $(mkdir_p) is intended to mean 'mkdir -p' I think it's more consistent for the expansion to not include the '--'. I think it is more practical to have $(mkdir_p) set to 'mkdir -p --'. And you can often write $(mkdir_p) $(directory_list) without knowing exactly the names of the directories. It's natural to forget that the directory name might start with a dash. And $(mkdir_p) cannot be used with an option anyway. So I propose the following variation of your patch. (The install-sh part remains unchanged.) Have a nice day, Stepan 2006-04-19 Paul Eggert [EMAIL PROTECTED] and Stepan Kasal [EMAIL PROTECTED] * lib/install.sh: Handle --, and diagnose unknown options. * m4/mkdirp.m4 (AM_PROG_MKDIR_P): Use '$(install_sh) -d --' as the fallback, for consistency with 'mkdir -p --'. Index: lib/install-sh === RCS file: /cvs/automake/automake/lib/install-sh,v retrieving revision 1.29 diff -u -r1.29 install-sh --- lib/install-sh 12 Jan 2006 21:11:14 - 1.29 +++ lib/install-sh 19 Apr 2006 11:04:28 - @@ -109,7 +109,7 @@ CHGRPPROG CHMODPROG CHOWNPROG CPPROG MKDIRPROG MVPROG RMPROG STRIPPROG -while test -n $1; do +while test $# -ne 0; do case $1 in -c) shift continue;; @@ -150,25 +150,33 @@ --version) echo $0 $scriptversion; exit $?;; -*) # When -d is used, all remaining arguments are directories to create. - # When -t is used, the destination is already specified. - test -n $dir_arg$dstarg break -# Otherwise, the last argument is the destination. Remove it from [EMAIL PROTECTED] - for arg - do - if test -n $dstarg; then - # $@ is not empty: it contains at least $arg. - set fnord $@ $dstarg - shift # fnord - fi - shift # arg - dstarg=$arg - done +--)shift break;; + +-*)echo $0: invalid option: $1 2 + exit 1;; + +*) break;; esac done -if test -z $1; then +if test $# -ne 0 test -z $dir_arg$dstarg; then + # When -d is used, all remaining arguments are directories to create. + # When -t is used, the destination is already specified. + # Otherwise, the last argument is the destination. Remove it from [EMAIL PROTECTED] + for arg + do +if test -n $dstarg; then + # $@ is not empty: it contains at least $arg. + set fnord $@ $dstarg + shift # fnord +fi +shift # arg +dstarg=$arg + done +fi + +if test $# -eq 0; then if test -z $dir_arg; then echo $0: no input file specified. 2 exit 1 Index: m4/mkdirp.m4 === RCS file: /cvs/automake/automake/m4/mkdirp.m4,v retrieving revision 1.8 diff -u -r1.8 mkdirp.m4 --- m4/mkdirp.m47 Aug 2005 08:10:06 - 1.8 +++ m4/mkdirp.m419 Apr 2006 11:04:30 - @@ -1,5 +1,5 @@ ## -*- Autoconf -*- -# Copyright (C) 2003, 2004, 2005 Free Software Foundation, Inc. +# Copyright (C) 2003, 2004, 2005, 2006 Free Software Foundation, Inc. # # This file is free software; the Free Software Foundation # gives unlimited permission to copy and/or distribute it, @@ -57,11 +57,6 @@ do test -d $d rmdir $d done - # $(mkinstalldirs) is defined by Automake if mkinstalldirs exists. - if test -f $ac_aux_dir/mkinstalldirs; then -mkdir_p='$(mkinstalldirs)' - else -mkdir_p='$(install_sh) -d' - fi + mkdir_p='$(install_sh) -d --' fi AC_SUBST([mkdir_p])])
Re: mkdir -p -- minor cleanup
Stepan Kasal [EMAIL PROTECTED] writes: I think it is more practical to have $(mkdir_p) set to 'mkdir -p --'. Yes, that is a possibility. And you can often write $(mkdir_p) $(directory_list) without knowing exactly the names of the directories. It's natural to forget that the directory name might start with a dash. But portable directory names cannot start with '-'. See http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap04.html#tag_04_06. I don't think we need to worry about applications that want to install files whose names begin with -. Such applications will already be in trouble, in lots of different ways. It should be a relatively minor burden on developers of such applications to use $(mkdir_p) -- instead of $(mkdir_p). And $(mkdir_p) cannot be used with an option anyway. But under the patch I proposed, $(mkdir_p) -m 444 would be allowed, for example. - # $(mkinstalldirs) is defined by Automake if mkinstalldirs exists. - if test -f $ac_aux_dir/mkinstalldirs; then -mkdir_p='$(mkinstalldirs)' - else -mkdir_p='$(install_sh) -d' - fi Hmm, why remove support for mkinstalldirs here? Isn't that a separate issue? I don't have a strong feeling for whether mkdir_p should end in ' --', or not. But it should be consistent, whichever way is chosen, so clearly a change is needed. A minor advantage of not ending in ' --' is that the output of make is easier to read.
Re: mkdir -p -- minor cleanup
PE == Paul Eggert [EMAIL PROTECTED] writes: PE Here's a proposed Automake patch to implement this. I noticed that PE install-sh handles neither -- nor unknown options, so I added support PE for that as well. Thanks, please install! -- Alexandre Duret-Lutz Shared books are happy books. http://www.bookcrossing.com/friend/gadl
Re: mkdir -p -- minor cleanup
Hello, On Wed, Apr 19, 2006 at 01:02:30AM -0700, Paul Eggert wrote: [...], but now that $(mkdir_p) is intended to mean 'mkdir -p' I think it's more consistent for the expansion to not include the '--'. I think it is more practical to have $(mkdir_p) set to 'mkdir -p --'. And you can often write $(mkdir_p) $(directory_list) without knowing exactly the names of the directories. It's natural to forget that the directory name might start with a dash. And $(mkdir_p) cannot be used with an option anyway. So I propose the following variation of your patch. (The install-sh part remains unchanged.) Have a nice day, Stepan 2006-04-19 Paul Eggert [EMAIL PROTECTED] and Stepan Kasal [EMAIL PROTECTED] * lib/install.sh: Handle --, and diagnose unknown options. * m4/mkdirp.m4 (AM_PROG_MKDIR_P): Use '$(install_sh) -d --' as the fallback, for consistency with 'mkdir -p --'. Index: lib/install-sh === RCS file: /cvs/automake/automake/lib/install-sh,v retrieving revision 1.29 diff -u -r1.29 install-sh --- lib/install-sh 12 Jan 2006 21:11:14 - 1.29 +++ lib/install-sh 19 Apr 2006 11:04:28 - @@ -109,7 +109,7 @@ CHGRPPROG CHMODPROG CHOWNPROG CPPROG MKDIRPROG MVPROG RMPROG STRIPPROG -while test -n $1; do +while test $# -ne 0; do case $1 in -c) shift continue;; @@ -150,25 +150,33 @@ --version) echo $0 $scriptversion; exit $?;; -*) # When -d is used, all remaining arguments are directories to create. - # When -t is used, the destination is already specified. - test -n $dir_arg$dstarg break -# Otherwise, the last argument is the destination. Remove it from [EMAIL PROTECTED] - for arg - do - if test -n $dstarg; then - # $@ is not empty: it contains at least $arg. - set fnord $@ $dstarg - shift # fnord - fi - shift # arg - dstarg=$arg - done +--)shift break;; + +-*)echo $0: invalid option: $1 2 + exit 1;; + +*) break;; esac done -if test -z $1; then +if test $# -ne 0 test -z $dir_arg$dstarg; then + # When -d is used, all remaining arguments are directories to create. + # When -t is used, the destination is already specified. + # Otherwise, the last argument is the destination. Remove it from [EMAIL PROTECTED] + for arg + do +if test -n $dstarg; then + # $@ is not empty: it contains at least $arg. + set fnord $@ $dstarg + shift # fnord +fi +shift # arg +dstarg=$arg + done +fi + +if test $# -eq 0; then if test -z $dir_arg; then echo $0: no input file specified. 2 exit 1 Index: m4/mkdirp.m4 === RCS file: /cvs/automake/automake/m4/mkdirp.m4,v retrieving revision 1.8 diff -u -r1.8 mkdirp.m4 --- m4/mkdirp.m47 Aug 2005 08:10:06 - 1.8 +++ m4/mkdirp.m419 Apr 2006 11:04:30 - @@ -1,5 +1,5 @@ ## -*- Autoconf -*- -# Copyright (C) 2003, 2004, 2005 Free Software Foundation, Inc. +# Copyright (C) 2003, 2004, 2005, 2006 Free Software Foundation, Inc. # # This file is free software; the Free Software Foundation # gives unlimited permission to copy and/or distribute it, @@ -57,11 +57,6 @@ do test -d $d rmdir $d done - # $(mkinstalldirs) is defined by Automake if mkinstalldirs exists. - if test -f $ac_aux_dir/mkinstalldirs; then -mkdir_p='$(mkinstalldirs)' - else -mkdir_p='$(install_sh) -d' - fi + mkdir_p='$(install_sh) -d --' fi AC_SUBST([mkdir_p])])
Re: mkdir -p -- minor cleanup
Stepan Kasal [EMAIL PROTECTED] writes: I think it is more practical to have $(mkdir_p) set to 'mkdir -p --'. Yes, that is a possibility. And you can often write $(mkdir_p) $(directory_list) without knowing exactly the names of the directories. It's natural to forget that the directory name might start with a dash. But portable directory names cannot start with '-'. See http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap04.html#tag_04_06. I don't think we need to worry about applications that want to install files whose names begin with -. Such applications will already be in trouble, in lots of different ways. It should be a relatively minor burden on developers of such applications to use $(mkdir_p) -- instead of $(mkdir_p). And $(mkdir_p) cannot be used with an option anyway. But under the patch I proposed, $(mkdir_p) -m 444 would be allowed, for example. - # $(mkinstalldirs) is defined by Automake if mkinstalldirs exists. - if test -f $ac_aux_dir/mkinstalldirs; then -mkdir_p='$(mkinstalldirs)' - else -mkdir_p='$(install_sh) -d' - fi Hmm, why remove support for mkinstalldirs here? Isn't that a separate issue? I don't have a strong feeling for whether mkdir_p should end in ' --', or not. But it should be consistent, whichever way is chosen, so clearly a change is needed. A minor advantage of not ending in ' --' is that the output of make is easier to read.
