GNOME Bugzilla – Bug 728506
build: add AC_PROG_SED so we can define $SED
Last modified: 2019-02-22 11:58:03 UTC
Created attachment 274677 [details] [review] add and use AC_PROG_SED Hi. Under at least OpenBSD and NetBSD, sed(1) does not support the `-i' switch for inplace editing, but we can force them to use GNU sed by setting SED to "gsed".
In the past we've tried to work around these issues by removing the use of -i. It's almost always unsafe when used in a Makefile anyway... The typical idiom (as here) of: somecmd -o out sed -i .... out is better replaced with somecmd -o out.tmp && sed ... < out.tmp > out.tmp2 && mv out.tmp2 out && rm -f out.tmp out.tmp2 owing to the fact that with this approach, a failure part way through won't leave half-baked files around.
Created attachment 274731 [details] [review] don't use non-portable sed -i shell command Hi Ryan. Something like that maybe?
Hi Ryan. Any feedback? Thanks.
Review of attachment 274731 [details] [review]: Please separate the unrelated cleanups to a separate patch. It's not really my place to approve patches to this module... ::: gcr/Makefile.am @@ +164,3 @@ + $(AM_V_GEN) sed -e 's/gcr_dbus/_gcr_dbus/g' \ + -e 's/temp-dbus-generated.h/gcr-dbus-generated.h/g' \ + gcr/temp-dbus-generated.c > gcr/gcr-dbus-generated.c This still doesn't follow the 'atomic' pattern of using mv to put the file in place only after it has been completely successfully created. I wouldn't worry about splitting the 'temp' part out to a separate rule. Just do it all as one shell fragment.
Hi Ryan. > Please separate the unrelated cleanups to a separate patch. What do you mean by unrelated? These files must be cleaned because of this patch no? > It's not really my place to approve patches to this module... > > ::: gcr/Makefile.am > @@ +164,3 @@ > + $(AM_V_GEN) sed -e 's/gcr_dbus/_gcr_dbus/g' \ > + -e 's/temp-dbus-generated.h/gcr-dbus-generated.h/g' \ > + gcr/temp-dbus-generated.c > gcr/gcr-dbus-generated.c > > This still doesn't follow the 'atomic' pattern of using mv to put the file in > place only after it has been completely successfully created. > > I wouldn't worry about splitting the 'temp' part out to a separate rule. Just > do it all as one shell fragment. Well I really tried to match your libsecret commit https://git.gnome.org/browse/libsecret/commit/?id=8d297361de86da651bf9caf809cf3f778cfad10e because you told me about it; so the 'sed -e' line is similar and so is the temp part.. If there is something I missed, please amend the patch, it will be simpler. Thanks.
Hi guys. Any other input on this sed patch? Thanks.
Here's how the atomic pattern works: sed ... > $(OUT).tmp && mv $(OUT).tmp $(OUT) Obviously replace $(OUT) with the variable or literal of the output file name. I agree with Ryan, that change would be good before we commit this patch.
Created attachment 276926 [details] [review] don't use non-portable sed -i shell command v2 Hopefully this is what you guys had in mind...
Thanks. Pushed to git master.