Parsing `ls` is fragile at best and _will_ fail when $tree
contains spaces. Replace this with a find-generated string
and directly assign it to $ALLSOURCE_ARCHS; a subshell is
implied by $(), so `cd` doesn't affect the current working
directory.
Signed-off-by: Joey Pabalinas <[email protected]>
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/scripts/tags.sh b/scripts/tags.sh
index 78e546ff689c2d5f40..c08347fdeef12a7621 100755
--- a/scripts/tags.sh
+++ b/scripts/tags.sh
@@ -26,24 +26,15 @@ else
fi
# ignore userspace tools
ignore="$ignore ( -path ${tree}tools ) -prune -o"
-# Find all available archs
-find_all_archs()
-{
- ALLSOURCE_ARCHS=""
- for arch in `ls ${tree}arch`; do
- ALLSOURCE_ARCHS="${ALLSOURCE_ARCHS} "${arch##\/}
- done
-}
-
# Detect if ALLSOURCE_ARCHS is set. If not, we assume SRCARCH
if [ "${ALLSOURCE_ARCHS}" = "" ]; then
ALLSOURCE_ARCHS=${SRCARCH}
elif [ "${ALLSOURCE_ARCHS}" = "all" ]; then
- find_all_archs
+ ALLSOURCE_ARCHS="$(find "${tree}arch/" -mindepth 1 -maxdepth 1 -type d -printf ' %f')"
fi
# find sources in arch/$ARCH
find_arch_sources()
{
--
2.17.0.rc1.35.g90bbd502d54fe92035.dirty
Hi.
The commit log is wrong.
2018-05-18 20:56 GMT+09:00 Joey Pabalinas <[email protected]>:
> Parsing `ls` is fragile at best and _will_ fail when $tree
> contains spaces.
This statement is wrong.
The cause of the problem is not using whatever command you use,
but missing quoting.
The following would work even if $tree contains spaces:
for arch in `ls "${tree}arch"`; do
BTW, what was your motivation of this patch?
Does ${tree} contain spaces?
If the file path contains spaces, the top Makefile terminates it earlier.
Makefile:128: *** main directory cannot contain spaces nor colons. Stop.
> Replace this with a find-generated string
> and directly assign it to $ALLSOURCE_ARCHS; a subshell is
> implied by $(), so `cd` doesn't affect the current working
> directory.
This patch no longer uses `cd`
> Signed-off-by: Joey Pabalinas <[email protected]>
>
> 1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/scripts/tags.sh b/scripts/tags.sh
> index 78e546ff689c2d5f40..c08347fdeef12a7621 100755
> --- a/scripts/tags.sh
> +++ b/scripts/tags.sh
> @@ -26,24 +26,15 @@ else
> fi
>
> # ignore userspace tools
> ignore="$ignore ( -path ${tree}tools ) -prune -o"
>
> -# Find all available archs
> -find_all_archs()
> -{
> - ALLSOURCE_ARCHS=""
> - for arch in `ls ${tree}arch`; do
> - ALLSOURCE_ARCHS="${ALLSOURCE_ARCHS} "${arch##\/}
> - done
> -}
> -
> # Detect if ALLSOURCE_ARCHS is set. If not, we assume SRCARCH
> if [ "${ALLSOURCE_ARCHS}" = "" ]; then
> ALLSOURCE_ARCHS=${SRCARCH}
> elif [ "${ALLSOURCE_ARCHS}" = "all" ]; then
> - find_all_archs
> + ALLSOURCE_ARCHS="$(find "${tree}arch/" -mindepth 1 -maxdepth 1 -type d -printf ' %f')"
> fi
>
> # find sources in arch/$ARCH
> find_arch_sources()
> {
> --
> 2.17.0.rc1.35.g90bbd502d54fe92035.dirty
>
--
Best Regards
Masahiro Yamada
On Tue, May 22, 2018 at 03:01:07PM +0900, Masahiro Yamada wrote:
> The commit log is wrong.
>
>
> 2018-05-18 20:56 GMT+09:00 Joey Pabalinas <[email protected]>:
> > Parsing `ls` is fragile at best and _will_ fail when $tree
> > contains spaces.
>
> This statement is wrong.
>
> The cause of the problem is not using whatever command you use,
> but missing quoting.
> The following would work even if $tree contains spaces:
>
> for arch in `ls "${tree}arch"`; do
Ah, to be completely honest that case didn't even occur to me.
> BTW, what was your motivation of this patch?
>
> Does ${tree} contain spaces?
>
> If the file path contains spaces, the top Makefile terminates it earlier.
>
> Makefile:128: *** main directory cannot contain spaces nor colons. Stop.
It doesn't; I didn't realize the Makefile already had a guard against
spaces in paths. It was something I noticed when poking at something
else and I thought it might be something worth fixing.
But now I agree with you that this patch isn't really needed at all. I can
no longer think of a case where even the original
> for arch in `ls ${tree}arch`; do
would break since spaces are not allowed at all.
Well, on the bright side, the times you happen to be wrong are also the
times where you learn the most :)
--
Cheers,
Joey Pabalinas
2018-05-22 17:21 GMT+09:00 Joey Pabalinas <[email protected]>:
> On Tue, May 22, 2018 at 03:01:07PM +0900, Masahiro Yamada wrote:
>> The commit log is wrong.
>>
>>
>> 2018-05-18 20:56 GMT+09:00 Joey Pabalinas <[email protected]>:
>> > Parsing `ls` is fragile at best and _will_ fail when $tree
>> > contains spaces.
>>
>> This statement is wrong.
>>
>> The cause of the problem is not using whatever command you use,
>> but missing quoting.
>> The following would work even if $tree contains spaces:
>>
>> for arch in `ls "${tree}arch"`; do
>
> Ah, to be completely honest that case didn't even occur to me.
>
>> BTW, what was your motivation of this patch?
>>
>> Does ${tree} contain spaces?
>
>>
>> If the file path contains spaces, the top Makefile terminates it earlier.
>>
>> Makefile:128: *** main directory cannot contain spaces nor colons. Stop.
>
> It doesn't; I didn't realize the Makefile already had a guard against
> spaces in paths. It was something I noticed when poking at something
> else and I thought it might be something worth fixing.
>
> But now I agree with you that this patch isn't really needed at all. I can
> no longer think of a case where even the original
>
>> for arch in `ls ${tree}arch`; do
>
> would break since spaces are not allowed at all.
>
> Well, on the bright side, the times you happen to be wrong are also the
> times where you learn the most :)
>
I see some new motivations for this patch now.
- The current code includes 'Kconfig' in ALLSOURCE_ARCHS,
but it should not.
- Simplify the code, removing the find_all_archs().
If you are motivated for v5,
how about this?
- Remove the double-quotes from "${tree}arch/"
(Because the rest parts of this script do not have double-quoting,
I do not see point in adding it in this line only.)
- Update the git-log to not mention the hypothetical space things.
--
Best Regards
Masahiro Yamada
On Tue, May 22, 2018 at 05:41:48PM +0900, Masahiro Yamada wrote:
> I see some new motivations for this patch now.
>
> - The current code includes 'Kconfig' in ALLSOURCE_ARCHS,
> but it should not.
>
> - Simplify the code, removing the find_all_archs().
>
>
> If you are motivated for v5,
> how about this?
>
> - Remove the double-quotes from "${tree}arch/"
> (Because the rest parts of this script do not have double-quoting,
> I do not see point in adding it in this line only.)
>
> - Update the git-log to not mention the hypothetical space things.
Alright, that sounds reasonable. What do you think of:
> ALLSOURCE_ARCHS=$(find ${tree}arch/ -mindepth 1 -maxdepth 1 -type d -printf '%f ')
The double quotes surrounding the arguments have been removed to be more
consistent with the other find commands in the script.
The ' %f' was changed to '%f ' so that you don't get that weird leading
space; I couldn't think of a way this could break anything but I wanted
to be 100% sure instead of needing to make a v6, heh.
The commit message will definitely be fixed as well.
--
Cheers,
Joey Pabalinas
2018-05-22 18:06 GMT+09:00 Joey Pabalinas <[email protected]>:
> On Tue, May 22, 2018 at 05:41:48PM +0900, Masahiro Yamada wrote:
>> I see some new motivations for this patch now.
>>
>> - The current code includes 'Kconfig' in ALLSOURCE_ARCHS,
>> but it should not.
>>
>> - Simplify the code, removing the find_all_archs().
>>
>>
>> If you are motivated for v5,
>> how about this?
>>
>> - Remove the double-quotes from "${tree}arch/"
>> (Because the rest parts of this script do not have double-quoting,
>> I do not see point in adding it in this line only.)
>>
>> - Update the git-log to not mention the hypothetical space things.
>
> Alright, that sounds reasonable. What do you think of:
>
>> ALLSOURCE_ARCHS=$(find ${tree}arch/ -mindepth 1 -maxdepth 1 -type d -printf '%f ')
>
> The double quotes surrounding the arguments have been removed to be more
> consistent with the other find commands in the script.
>
> The ' %f' was changed to '%f ' so that you don't get that weird leading
> space; I couldn't think of a way this could break anything but I wanted
> to be 100% sure instead of needing to make a v6, heh.
Fine with me.
--
Best Regards
Masahiro Yamada