| Message ID | 20190502173327.23285-1-jeffery.to@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Petr Štetiar |
| Headers | show |
| Series | [OpenWrt-Devel] build: fix STAGING_DIR cleaning when filenames contain spaces | expand |
Jeffery To <jeffery.to@gmail.com> [2019-05-03 01:33:27]: > When looping through a package's STAGING_FILES_LIST (a list of > file/directory paths delimited by newlines), if the path contains > spaces, then the path will be split by the while loops, and the > file/directory will not be deleted/removed. > > This sets the internal field separator to the newline only so that the > entire path is considered when deleting/removing. > > Signed-off-by: Jeffery To <jeffery.to@gmail.com> > --- > scripts/clean-package.sh | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/scripts/clean-package.sh b/scripts/clean-package.sh > index e580566a52..3a824929c6 100755 > --- a/scripts/clean-package.sh > +++ b/scripts/clean-package.sh > @@ -1,4 +1,6 @@ > #!/usr/bin/env bash > +IFS=" > +" > [ -n "$1" -a -n "$2" ] || { > echo "Usage: $0 <file> <directory>" > exit 1 just for the record, copy&pasting my comment from your PR[1]: I'm just wondering, if this proposed fix with this strange looking IFS is the right direction. Usually if I've problem with whitespaces in filenames, it's because I've forget to use xargs. If I'm not mistaken, this [ -n "$entry" ] || break construct could be replaced with xargs -r, there's even XARGS:=xargs -r in rules.mk. Just my two cents. 1. https://github.com/openwrt/openwrt/pull/1806#issuecomment-475454138 -- ynezz
On Fri, May 3, 2019 at 5:40 PM Petr Štetiar <ynezz@true.cz> wrote: > Jeffery To <jeffery.to@gmail.com> [2019-05-03 01:33:27]: > > > When looping through a package's STAGING_FILES_LIST (a list of > > file/directory paths delimited by newlines), if the path contains > > spaces, then the path will be split by the while loops, and the > > file/directory will not be deleted/removed. > > > > This sets the internal field separator to the newline only so that the > > entire path is considered when deleting/removing. > > > > Signed-off-by: Jeffery To <jeffery.to@gmail.com> > > --- > > scripts/clean-package.sh | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/scripts/clean-package.sh b/scripts/clean-package.sh > > index e580566a52..3a824929c6 100755 > > --- a/scripts/clean-package.sh > > +++ b/scripts/clean-package.sh > > @@ -1,4 +1,6 @@ > > #!/usr/bin/env bash > > +IFS=" > > +" > > [ -n "$1" -a -n "$2" ] || { > > echo "Usage: $0 <file> <directory>" > > exit 1 > > just for the record, copy&pasting my comment from your PR[1]: > > I'm just wondering, if this proposed fix with this strange looking IFS is > the > right direction. Usually if I've problem with whitespaces in filenames, > it's > because I've forget to use xargs. If I'm not mistaken, this [ -n "$entry" > ] || > break construct could be replaced with xargs -r, there's even XARGS:=xargs > -r > in rules.mk. Just my two cents. > > 1. https://github.com/openwrt/openwrt/pull/1806#issuecomment-475454138 > > -- ynezz > For the record, here is my reply from the PR[1]: IFS[2] is a standard internal variable that controls how the shell splits tokens. Setting it here determines how the while loops further down the script will split the input (lines of file paths generated by find). Normally, IFS contains space, newline, and tab, which causes the while loops to incorrectly split paths with spaces. This problem is avoided by setting IFS to the newline only. The line you cite ([ -n "$entry" ] || break) causes the while loops (which delete the input file paths) to exit on the first empty line of input. It would make no sense to replace it with xargs. * * * I finally remembered, I copied this syntax from package/network/services/openvpn/files/openvpn.init[3]: LIST_SEP=" " then further down it's used in append_param()[4] as IFS="$LIST_SEP" (I won't quote the whole for loop here). If you prefer, I can change this to use ksh93 syntax[5]: IFS=$'\n' If you want to rewrite the script to use xargs, please be my guest, but setting IFS solves the issue here of spaces in file paths (technically, lines of text read from a file list). 1. https://github.com/openwrt/openwrt/pull/1806#issuecomment-475506440 2. https://www.tldp.org/LDP/abs/html/internalvariables.html#IFSREF 3. https://github.com/openwrt/openwrt/blob/172b02c05f2dc199f511f1577aeddfabb7790dc0/package/network/services/openvpn/files/openvpn.init#L13-L14 4. https://github.com/openwrt/openwrt/blob/172b02c05f2dc199f511f1577aeddfabb7790dc0/package/network/services/openvpn/files/openvpn.init#L43 5. https://unix.stackexchange.com/a/184867
On Wed, May 15, 2019 at 7:03 PM Jeffery To <jeffery.to@gmail.com> wrote: > On Fri, May 3, 2019 at 5:40 PM Petr Štetiar <ynezz@true.cz> wrote: > >> Jeffery To <jeffery.to@gmail.com> [2019-05-03 01:33:27]: >> >> > When looping through a package's STAGING_FILES_LIST (a list of >> > file/directory paths delimited by newlines), if the path contains >> > spaces, then the path will be split by the while loops, and the >> > file/directory will not be deleted/removed. >> > >> > This sets the internal field separator to the newline only so that the >> > entire path is considered when deleting/removing. >> > >> > Signed-off-by: Jeffery To <jeffery.to@gmail.com> >> > --- >> > scripts/clean-package.sh | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/scripts/clean-package.sh b/scripts/clean-package.sh >> > index e580566a52..3a824929c6 100755 >> > --- a/scripts/clean-package.sh >> > +++ b/scripts/clean-package.sh >> > @@ -1,4 +1,6 @@ >> > #!/usr/bin/env bash >> > +IFS=" >> > +" >> > [ -n "$1" -a -n "$2" ] || { >> > echo "Usage: $0 <file> <directory>" >> > exit 1 >> >> just for the record, copy&pasting my comment from your PR[1]: >> >> I'm just wondering, if this proposed fix with this strange looking IFS is >> the >> right direction. Usually if I've problem with whitespaces in filenames, >> it's >> because I've forget to use xargs. If I'm not mistaken, this [ -n "$entry" >> ] || >> break construct could be replaced with xargs -r, there's even >> XARGS:=xargs -r >> in rules.mk. Just my two cents. >> >> 1. https://github.com/openwrt/openwrt/pull/1806#issuecomment-475454138 >> >> -- ynezz >> > > For the record, here is my reply from the PR[1]: > > IFS[2] is a standard internal variable that controls how the shell splits > tokens. Setting it here determines how the while loops further down the > script will split the input (lines of file paths generated by find). > Normally, IFS contains space, newline, and tab, which causes the while > loops to incorrectly split paths with spaces. This problem is avoided by > setting IFS to the newline only. > > The line you cite ([ -n "$entry" ] || break) causes the while loops (which > delete the input file paths) to exit on the first empty line of input. It > would make no sense to replace it with xargs. > > * * * > > I finally remembered, I copied this syntax from > package/network/services/openvpn/files/openvpn.init[3]: > > LIST_SEP=" > " > > then further down it's used in append_param()[4] as IFS="$LIST_SEP" (I > won't quote the whole for loop here). > > If you prefer, I can change this to use ksh93 syntax[5]: > > IFS=$'\n' > > If you want to rewrite the script to use xargs, please be my guest, but > setting IFS solves the issue here of spaces in file paths (technically, > lines of text read from a file list). > > > 1. https://github.com/openwrt/openwrt/pull/1806#issuecomment-475506440 > 2. https://www.tldp.org/LDP/abs/html/internalvariables.html#IFSREF > 3. > https://github.com/openwrt/openwrt/blob/172b02c05f2dc199f511f1577aeddfabb7790dc0/package/network/services/openvpn/files/openvpn.init#L13-L14 > 4. > https://github.com/openwrt/openwrt/blob/172b02c05f2dc199f511f1577aeddfabb7790dc0/package/network/services/openvpn/files/openvpn.init#L43 > 5. https://unix.stackexchange.com/a/184867 > > Just want to add, it may not be obvious why there would be spaces in file paths (it wasn't obvious to me at first). Apparently, some Go packages that users have been trying to build (Docker[1], Delve[2]) include files with spaces in their names (mostly as test cases for handling files with spaces in their name, I think). The Go compilation issue was eventually fixed, but I noticed that the files were not cleaned up properly from STAGING_DIR. 1. https://github.com/openwrt/packages/pull/7127#issuecomment-439688448 2. https://github.com/openwrt/packages/issues/7639
Jeffery To <jeffery.to@gmail.com> [2019-05-15 19:03:16]: > I finally remembered, I copied this syntax from > package/network/services/openvpn/files/openvpn.init[3]: > > LIST_SEP=" > " it just looks weird as well. > If you prefer, I can change this to use ksh93 syntax[5]: > > IFS=$'\n' indeed, I like this a lot more, and IFS="$(printf '\n\t')" from that TLDP page you've linked looks good as well, seems even more readable to me. Anyway, I'll leave the decision up to you, both are fine with me. -- ynezz
On Wed, May 15, 2019 at 9:24 PM Petr Štetiar <ynezz@true.cz> wrote: > Jeffery To <jeffery.to@gmail.com> [2019-05-15 19:03:16]: > > > If you prefer, I can change this to use ksh93 syntax[5]: > > > > IFS=$'\n' > > indeed, I like this a lot more, and > > IFS="$(printf '\n\t')" > > from that TLDP page you've linked looks good as well, seems even more > readable > to me. Anyway, I'll leave the decision up to you, both are fine with me. > Sure, I'll change it to IFS=$'\n'.
diff --git a/scripts/clean-package.sh b/scripts/clean-package.sh index e580566a52..3a824929c6 100755 --- a/scripts/clean-package.sh +++ b/scripts/clean-package.sh @@ -1,4 +1,6 @@ #!/usr/bin/env bash +IFS=" +" [ -n "$1" -a -n "$2" ] || { echo "Usage: $0 <file> <directory>" exit 1
When looping through a package's STAGING_FILES_LIST (a list of file/directory paths delimited by newlines), if the path contains spaces, then the path will be split by the while loops, and the file/directory will not be deleted/removed. This sets the internal field separator to the newline only so that the entire path is considered when deleting/removing. Signed-off-by: Jeffery To <jeffery.to@gmail.com> --- scripts/clean-package.sh | 2 ++ 1 file changed, 2 insertions(+)