| Message ID | 20190927093603.9140-8-yamada.masahiro@socionext.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | module: various bug-fixes and clean-ups for module namespace | expand |
On Fri, Sep 27, 2019 at 06:36:03PM +0900, Masahiro Yamada wrote: >scripts/nsdeps automatically generates a patch to add MODULE_IMPORT_NS >tags, and what is nicer, it sorts the lines alphabetically with the >"sort" command. However, the output from the "sort" command depends >on locale. > >Especially when namespaces contain underscores, the result is >different depending on the locale. > >For example, I got this: > >$ { echo usbcommon; echo usb_common; } | LANG=en_US.UTF-8 sort >usbcommon >usb_common >$ { echo usbcommon; echo usb_common; } | LANG=C sort >usb_common >usbcommon > >So, this means people might potentially send different patches. > >This kind of issue was reported in the past, for example, >commit f55f2328bb28 ("kbuild: make sorting initramfs contents >independent of locale"). > >Adding "LANG=C" is a conventional way of fixing when a deterministic >result is desirable. > >Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >--- > > scripts/nsdeps | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/scripts/nsdeps b/scripts/nsdeps >index 964b7fb8c546..3754dac13b31 100644 >--- a/scripts/nsdeps >+++ b/scripts/nsdeps >@@ -41,7 +41,7 @@ generate_deps() { > for source_file in $mod_source_files; do > sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp > offset=$(wc -l ${source_file}.tmp | awk '{print $1;}') >- cat $source_file | grep MODULE_IMPORT_NS | sort -u >> ${source_file}.tmp >+ cat $source_file | grep MODULE_IMPORT_NS | LANG=C sort -u >> ${source_file}.tmp I would prefer to have this set throughout the whole runtime of the script. Otherwise we likely see a followup patch. So, either as an export at the beginning of this file or as part of the command that calls this script. With this Reviewed-by: Matthias Maennich <maennich@google.com> Cheers, Matthias > tail -n +$((offset +1)) ${source_file} | grep -v MODULE_IMPORT_NS >> ${source_file}.tmp > if ! diff -q ${source_file} ${source_file}.tmp; then > mv ${source_file}.tmp ${source_file} >-- >2.17.1 >
On Fri, Sep 27, 2019 at 10:27 PM Matthias Maennich <maennich@google.com> wrote: > > On Fri, Sep 27, 2019 at 06:36:03PM +0900, Masahiro Yamada wrote: > >scripts/nsdeps automatically generates a patch to add MODULE_IMPORT_NS > >tags, and what is nicer, it sorts the lines alphabetically with the > >"sort" command. However, the output from the "sort" command depends > >on locale. > > > >Especially when namespaces contain underscores, the result is > >different depending on the locale. > > > >For example, I got this: > > > >$ { echo usbcommon; echo usb_common; } | LANG=en_US.UTF-8 sort > >usbcommon > >usb_common > >$ { echo usbcommon; echo usb_common; } | LANG=C sort > >usb_common > >usbcommon > > > >So, this means people might potentially send different patches. > > > >This kind of issue was reported in the past, for example, > >commit f55f2328bb28 ("kbuild: make sorting initramfs contents > >independent of locale"). > > > >Adding "LANG=C" is a conventional way of fixing when a deterministic > >result is desirable. > > > >Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > >--- > > > > scripts/nsdeps | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > >diff --git a/scripts/nsdeps b/scripts/nsdeps > >index 964b7fb8c546..3754dac13b31 100644 > >--- a/scripts/nsdeps > >+++ b/scripts/nsdeps > >@@ -41,7 +41,7 @@ generate_deps() { > > for source_file in $mod_source_files; do > > sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp > > offset=$(wc -l ${source_file}.tmp | awk '{print $1;}') > >- cat $source_file | grep MODULE_IMPORT_NS | sort -u >> ${source_file}.tmp > >+ cat $source_file | grep MODULE_IMPORT_NS | LANG=C sort -u >> ${source_file}.tmp > > I would prefer to have this set throughout the whole runtime of the > script. Otherwise we likely see a followup patch. So, either as an > export at the beginning of this file or as part of the command that > calls this script. I prefer to keep it close to the locale-dependent code. If I move it to somewhere else, I need to add a comment like # make "sort" command deterministic export LANG=C Otherwise, people would have no idea why it is needed. > With this > > Reviewed-by: Matthias Maennich <maennich@google.com> > > Cheers, > Matthias > > > tail -n +$((offset +1)) ${source_file} | grep -v MODULE_IMPORT_NS >> ${source_file}.tmp > > if ! diff -q ${source_file} ${source_file}.tmp; then > > mv ${source_file}.tmp ${source_file} > >-- > >2.17.1 > > -- Best Regards Masahiro Yamada
On Sat, Sep 28, 2019 at 12:42:28AM +0900, Masahiro Yamada wrote: > On Fri, Sep 27, 2019 at 10:27 PM Matthias Maennich <maennich@google.com> wrote: > > > > On Fri, Sep 27, 2019 at 06:36:03PM +0900, Masahiro Yamada wrote: > > >scripts/nsdeps automatically generates a patch to add MODULE_IMPORT_NS > > >tags, and what is nicer, it sorts the lines alphabetically with the > > >"sort" command. However, the output from the "sort" command depends > > >on locale. > > > > > >Especially when namespaces contain underscores, the result is > > >different depending on the locale. > > > > > >For example, I got this: > > > > > >$ { echo usbcommon; echo usb_common; } | LANG=en_US.UTF-8 sort > > >usbcommon > > >usb_common > > >$ { echo usbcommon; echo usb_common; } | LANG=C sort > > >usb_common > > >usbcommon > > > > > >So, this means people might potentially send different patches. > > > > > >This kind of issue was reported in the past, for example, > > >commit f55f2328bb28 ("kbuild: make sorting initramfs contents > > >independent of locale"). > > > > > >Adding "LANG=C" is a conventional way of fixing when a deterministic > > >result is desirable. > > > > > >Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > >--- > > > > > > scripts/nsdeps | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > >diff --git a/scripts/nsdeps b/scripts/nsdeps > > >index 964b7fb8c546..3754dac13b31 100644 > > >--- a/scripts/nsdeps > > >+++ b/scripts/nsdeps > > >@@ -41,7 +41,7 @@ generate_deps() { > > > for source_file in $mod_source_files; do > > > sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp > > > offset=$(wc -l ${source_file}.tmp | awk '{print $1;}') > > >- cat $source_file | grep MODULE_IMPORT_NS | sort -u >> ${source_file}.tmp > > >+ cat $source_file | grep MODULE_IMPORT_NS | LANG=C sort -u >> ${source_file}.tmp > > > > I would prefer to have this set throughout the whole runtime of the > > script. Otherwise we likely see a followup patch. So, either as an > > export at the beginning of this file or as part of the command that > > calls this script. > > > I prefer to keep it close to the locale-dependent code. > > > > If I move it to somewhere else, I need to add a comment like > > # make "sort" command deterministic > export LANG=C > > Otherwise, people would have no idea why it is needed. A comment is fine, it documents why it is here and it keeps anyone from having to remember to add it to anything else that changes in here. thanks, greg k-h
On Sat, Sep 28, 2019 at 3:14 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Sat, Sep 28, 2019 at 12:42:28AM +0900, Masahiro Yamada wrote: > > On Fri, Sep 27, 2019 at 10:27 PM Matthias Maennich <maennich@google.com> wrote: > > > > > > On Fri, Sep 27, 2019 at 06:36:03PM +0900, Masahiro Yamada wrote: > > > >scripts/nsdeps automatically generates a patch to add MODULE_IMPORT_NS > > > >tags, and what is nicer, it sorts the lines alphabetically with the > > > >"sort" command. However, the output from the "sort" command depends > > > >on locale. > > > > > > > >Especially when namespaces contain underscores, the result is > > > >different depending on the locale. > > > > > > > >For example, I got this: > > > > > > > >$ { echo usbcommon; echo usb_common; } | LANG=en_US.UTF-8 sort > > > >usbcommon > > > >usb_common > > > >$ { echo usbcommon; echo usb_common; } | LANG=C sort > > > >usb_common > > > >usbcommon > > > > > > > >So, this means people might potentially send different patches. > > > > > > > >This kind of issue was reported in the past, for example, > > > >commit f55f2328bb28 ("kbuild: make sorting initramfs contents > > > >independent of locale"). > > > > > > > >Adding "LANG=C" is a conventional way of fixing when a deterministic > > > >result is desirable. > > > > > > > >Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > >--- > > > > > > > > scripts/nsdeps | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > >diff --git a/scripts/nsdeps b/scripts/nsdeps > > > >index 964b7fb8c546..3754dac13b31 100644 > > > >--- a/scripts/nsdeps > > > >+++ b/scripts/nsdeps > > > >@@ -41,7 +41,7 @@ generate_deps() { > > > > for source_file in $mod_source_files; do > > > > sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp > > > > offset=$(wc -l ${source_file}.tmp | awk '{print $1;}') > > > >- cat $source_file | grep MODULE_IMPORT_NS | sort -u >> ${source_file}.tmp > > > >+ cat $source_file | grep MODULE_IMPORT_NS | LANG=C sort -u >> ${source_file}.tmp > > > > > > I would prefer to have this set throughout the whole runtime of the > > > script. Otherwise we likely see a followup patch. So, either as an > > > export at the beginning of this file or as part of the command that > > > calls this script. > > > > > > I prefer to keep it close to the locale-dependent code. > > > > > > > > If I move it to somewhere else, I need to add a comment like > > > > # make "sort" command deterministic > > export LANG=C > > > > Otherwise, people would have no idea why it is needed. > > A comment is fine, it documents why it is here and it keeps anyone from > having to remember to add it to anything else that changes in here. > > thanks, > > greg k-h Huh, people who live in a country with English as mother tongue cannot understand the i18n because English is the only language in the world? For example, in my locale (ja_JP.UTF-8) I can see messages in Japanese as follows: $ sh scripts/nsdeps cat: /modules.order: そのようなファイルやディレクトリはありません If I added "LANG=C" to the whole of this script, it would forcibly change all the messages into English. $ sh scripts/nsdeps cat: /modules.order: No such file or directory The impact on the locale should be really limited. So, "LANG=C" must be placed immediately before the "find" command. Nobody is asking to change log messages into English, and offering what was not asked is just *pushy*. -- Best Regards Masahiro Yamada
On Sun, Sep 29, 2019 at 10:18 AM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > On Sat, Sep 28, 2019 at 3:14 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Sat, Sep 28, 2019 at 12:42:28AM +0900, Masahiro Yamada wrote: > > > On Fri, Sep 27, 2019 at 10:27 PM Matthias Maennich <maennich@google.com> wrote: > > > > > > > > On Fri, Sep 27, 2019 at 06:36:03PM +0900, Masahiro Yamada wrote: > > > > >scripts/nsdeps automatically generates a patch to add MODULE_IMPORT_NS > > > > >tags, and what is nicer, it sorts the lines alphabetically with the > > > > >"sort" command. However, the output from the "sort" command depends > > > > >on locale. > > > > > > > > > >Especially when namespaces contain underscores, the result is > > > > >different depending on the locale. > > > > > > > > > >For example, I got this: > > > > > > > > > >$ { echo usbcommon; echo usb_common; } | LANG=en_US.UTF-8 sort > > > > >usbcommon > > > > >usb_common > > > > >$ { echo usbcommon; echo usb_common; } | LANG=C sort > > > > >usb_common > > > > >usbcommon > > > > > > > > > >So, this means people might potentially send different patches. > > > > > > > > > >This kind of issue was reported in the past, for example, > > > > >commit f55f2328bb28 ("kbuild: make sorting initramfs contents > > > > >independent of locale"). > > > > > > > > > >Adding "LANG=C" is a conventional way of fixing when a deterministic > > > > >result is desirable. > > > > > > > > > >Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > > >--- > > > > > > > > > > scripts/nsdeps | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > >diff --git a/scripts/nsdeps b/scripts/nsdeps > > > > >index 964b7fb8c546..3754dac13b31 100644 > > > > >--- a/scripts/nsdeps > > > > >+++ b/scripts/nsdeps > > > > >@@ -41,7 +41,7 @@ generate_deps() { > > > > > for source_file in $mod_source_files; do > > > > > sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp > > > > > offset=$(wc -l ${source_file}.tmp | awk '{print $1;}') > > > > >- cat $source_file | grep MODULE_IMPORT_NS | sort -u >> ${source_file}.tmp > > > > >+ cat $source_file | grep MODULE_IMPORT_NS | LANG=C sort -u >> ${source_file}.tmp > > > > > > > > I would prefer to have this set throughout the whole runtime of the > > > > script. Otherwise we likely see a followup patch. So, either as an > > > > export at the beginning of this file or as part of the command that > > > > calls this script. > > > > > > > > > I prefer to keep it close to the locale-dependent code. > > > > > > > > > > > > If I move it to somewhere else, I need to add a comment like > > > > > > # make "sort" command deterministic > > > export LANG=C > > > > > > Otherwise, people would have no idea why it is needed. > > > > A comment is fine, it documents why it is here and it keeps anyone from > > having to remember to add it to anything else that changes in here. > > > > thanks, > > > > greg k-h > > > Huh, people who live in a country with English as mother tongue > cannot understand the i18n because English is the > only language in the world? I take back this comment. I actually do not know where you live, or what native language you speak. It was used to exaggerate things, but It is not important to the point of discussions. Sorry. -- Best Regards Masahiro Yamada
On Sun, Sep 29, 2019 at 10:18:50AM +0900, Masahiro Yamada wrote: > On Sat, Sep 28, 2019 at 3:14 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Sat, Sep 28, 2019 at 12:42:28AM +0900, Masahiro Yamada wrote: > > > On Fri, Sep 27, 2019 at 10:27 PM Matthias Maennich <maennich@google.com> wrote: > > > > > > > > On Fri, Sep 27, 2019 at 06:36:03PM +0900, Masahiro Yamada wrote: > > > > >scripts/nsdeps automatically generates a patch to add MODULE_IMPORT_NS > > > > >tags, and what is nicer, it sorts the lines alphabetically with the > > > > >"sort" command. However, the output from the "sort" command depends > > > > >on locale. > > > > > > > > > >Especially when namespaces contain underscores, the result is > > > > >different depending on the locale. > > > > > > > > > >For example, I got this: > > > > > > > > > >$ { echo usbcommon; echo usb_common; } | LANG=en_US.UTF-8 sort > > > > >usbcommon > > > > >usb_common > > > > >$ { echo usbcommon; echo usb_common; } | LANG=C sort > > > > >usb_common > > > > >usbcommon > > > > > > > > > >So, this means people might potentially send different patches. > > > > > > > > > >This kind of issue was reported in the past, for example, > > > > >commit f55f2328bb28 ("kbuild: make sorting initramfs contents > > > > >independent of locale"). > > > > > > > > > >Adding "LANG=C" is a conventional way of fixing when a deterministic > > > > >result is desirable. > > > > > > > > > >Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > > >--- > > > > > > > > > > scripts/nsdeps | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > >diff --git a/scripts/nsdeps b/scripts/nsdeps > > > > >index 964b7fb8c546..3754dac13b31 100644 > > > > >--- a/scripts/nsdeps > > > > >+++ b/scripts/nsdeps > > > > >@@ -41,7 +41,7 @@ generate_deps() { > > > > > for source_file in $mod_source_files; do > > > > > sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp > > > > > offset=$(wc -l ${source_file}.tmp | awk '{print $1;}') > > > > >- cat $source_file | grep MODULE_IMPORT_NS | sort -u >> ${source_file}.tmp > > > > >+ cat $source_file | grep MODULE_IMPORT_NS | LANG=C sort -u >> ${source_file}.tmp > > > > > > > > I would prefer to have this set throughout the whole runtime of the > > > > script. Otherwise we likely see a followup patch. So, either as an > > > > export at the beginning of this file or as part of the command that > > > > calls this script. > > > > > > > > > I prefer to keep it close to the locale-dependent code. > > > > > > > > > > > > If I move it to somewhere else, I need to add a comment like > > > > > > # make "sort" command deterministic > > > export LANG=C > > > > > > Otherwise, people would have no idea why it is needed. > > > > A comment is fine, it documents why it is here and it keeps anyone from > > having to remember to add it to anything else that changes in here. > > > > thanks, > > > > greg k-h > > > Huh, people who live in a country with English as mother tongue > cannot understand the i18n because English is the > only language in the world? Heh, I live in a country where English is not the "mother tongue" and I totally missed this, sorry about that :( > For example, in my locale (ja_JP.UTF-8) > > I can see messages in Japanese as follows: > > $ sh scripts/nsdeps > cat: /modules.order: そのようなファイルやディレクトリはありません Ugh, I forgot this would change the error messages from other programs. You are right, your proposal isn't ok, I missed this given that I am used to programming in English. sorry, greg k-h
On Sun, Sep 29, 2019 at 10:30:27AM +0900, Masahiro Yamada wrote: >On Sun, Sep 29, 2019 at 10:18 AM Masahiro Yamada ><yamada.masahiro@socionext.com> wrote: >> >> On Sat, Sep 28, 2019 at 3:14 AM Greg Kroah-Hartman >> <gregkh@linuxfoundation.org> wrote: >> > >> > On Sat, Sep 28, 2019 at 12:42:28AM +0900, Masahiro Yamada wrote: >> > > On Fri, Sep 27, 2019 at 10:27 PM Matthias Maennich <maennich@google.com> wrote: >> > > > >> > > > On Fri, Sep 27, 2019 at 06:36:03PM +0900, Masahiro Yamada wrote: >> > > > >scripts/nsdeps automatically generates a patch to add MODULE_IMPORT_NS >> > > > >tags, and what is nicer, it sorts the lines alphabetically with the >> > > > >"sort" command. However, the output from the "sort" command depends >> > > > >on locale. >> > > > > >> > > > >Especially when namespaces contain underscores, the result is >> > > > >different depending on the locale. >> > > > > >> > > > >For example, I got this: >> > > > > >> > > > >$ { echo usbcommon; echo usb_common; } | LANG=en_US.UTF-8 sort >> > > > >usbcommon >> > > > >usb_common >> > > > >$ { echo usbcommon; echo usb_common; } | LANG=C sort >> > > > >usb_common >> > > > >usbcommon >> > > > > >> > > > >So, this means people might potentially send different patches. >> > > > > >> > > > >This kind of issue was reported in the past, for example, >> > > > >commit f55f2328bb28 ("kbuild: make sorting initramfs contents >> > > > >independent of locale"). >> > > > > >> > > > >Adding "LANG=C" is a conventional way of fixing when a deterministic >> > > > >result is desirable. >> > > > > >> > > > >Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >> > > > >--- >> > > > > >> > > > > scripts/nsdeps | 2 +- >> > > > > 1 file changed, 1 insertion(+), 1 deletion(-) >> > > > > >> > > > >diff --git a/scripts/nsdeps b/scripts/nsdeps >> > > > >index 964b7fb8c546..3754dac13b31 100644 >> > > > >--- a/scripts/nsdeps >> > > > >+++ b/scripts/nsdeps >> > > > >@@ -41,7 +41,7 @@ generate_deps() { >> > > > > for source_file in $mod_source_files; do >> > > > > sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp >> > > > > offset=$(wc -l ${source_file}.tmp | awk '{print $1;}') >> > > > >- cat $source_file | grep MODULE_IMPORT_NS | sort -u >> ${source_file}.tmp >> > > > >+ cat $source_file | grep MODULE_IMPORT_NS | LANG=C sort -u >> ${source_file}.tmp >> > > > >> > > > I would prefer to have this set throughout the whole runtime of the >> > > > script. Otherwise we likely see a followup patch. So, either as an >> > > > export at the beginning of this file or as part of the command that >> > > > calls this script. >> > > >> > > >> > > I prefer to keep it close to the locale-dependent code. >> > > >> > > >> > > >> > > If I move it to somewhere else, I need to add a comment like >> > > >> > > # make "sort" command deterministic >> > > export LANG=C >> > > >> > > Otherwise, people would have no idea why it is needed. >> > >> > A comment is fine, it documents why it is here and it keeps anyone from >> > having to remember to add it to anything else that changes in here. >> > >> > thanks, >> > >> > greg k-h >> >> >> Huh, people who live in a country with English as mother tongue >> cannot understand the i18n because English is the >> only language in the world? > >I take back this comment. >I actually do not know where you live, or what native language you speak. >It was used to exaggerate things, but It is not important to >the point of discussions. Sorry. Thanks for pointing this out and reminding us! I am not a native English speaker, but often are surrounded by English, especially when programming. Error messages in my native language feel often rather funny than helpful, but I guess this not the case for most translations. Based on that I am ok with your original version of the patch. Cheers, Matthias
diff --git a/scripts/nsdeps b/scripts/nsdeps index 964b7fb8c546..3754dac13b31 100644 --- a/scripts/nsdeps +++ b/scripts/nsdeps @@ -41,7 +41,7 @@ generate_deps() { for source_file in $mod_source_files; do sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp offset=$(wc -l ${source_file}.tmp | awk '{print $1;}') - cat $source_file | grep MODULE_IMPORT_NS | sort -u >> ${source_file}.tmp + cat $source_file | grep MODULE_IMPORT_NS | LANG=C sort -u >> ${source_file}.tmp tail -n +$((offset +1)) ${source_file} | grep -v MODULE_IMPORT_NS >> ${source_file}.tmp if ! diff -q ${source_file} ${source_file}.tmp; then mv ${source_file}.tmp ${source_file}
scripts/nsdeps automatically generates a patch to add MODULE_IMPORT_NS tags, and what is nicer, it sorts the lines alphabetically with the "sort" command. However, the output from the "sort" command depends on locale. Especially when namespaces contain underscores, the result is different depending on the locale. For example, I got this: $ { echo usbcommon; echo usb_common; } | LANG=en_US.UTF-8 sort usbcommon usb_common $ { echo usbcommon; echo usb_common; } | LANG=C sort usb_common usbcommon So, this means people might potentially send different patches. This kind of issue was reported in the past, for example, commit f55f2328bb28 ("kbuild: make sorting initramfs contents independent of locale"). Adding "LANG=C" is a conventional way of fixing when a deterministic result is desirable. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- scripts/nsdeps | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.17.1