Make sure LC_ALL=C is set in all shell scripts by practicalswift · Pull Request #13454 · bitcoin/bitcoin

practicalswift · 2018-06-13T05:55:25Z

~~Make sure LC_ALL=C is set when using grep range expressions.~~

Make sure LC_ALL=C is set in all shell scripts.

From the grep(1) documentation:

Within a bracket expression, a range expression consists of two characters separated by a hyphen. It matches any single character that sorts between the two characters, inclusive, using the locale's collating sequence and character set. For example, in the default C locale, [a-d] is equivalent to [abcd]. Many locales sort characters in dictionary order, and in these locales [a-d] is typically not equivalent to [abcd]; it might be equivalent to [aBbCcDd], for example. To obtain the traditional interpretation of bracket expressions, you can use the C locale by setting the LC_ALL environment variable to the value C.

Context: Locale issue found when reviewing #13450

laanwj · 2018-06-13T10:46:54Z

Maybe export LC_ALL=C at the beginning of these scripts, to rule out unexpected locale dependency in other commands too.
(this also avoids making the code look even more cluttered)

practicalswift · 2018-06-13T14:54:34Z

@laanwj Good idea. Now enforcing export LC_ALL=C. Please re-review :-)

maflcko · 2018-06-13T14:57:48Z

How would shellcheck segfault on this? 🤕

test/lint/lint-shell.sh: line 29:  8402 Segmentation fault      (core dumped) shellcheck -e SC2001,SC2004,SC2005,SC2006,SC2016,SC2028,SC2046,SC2048,SC2066,SC2086,SC2116,SC2148,SC2162,SC2166,SC2181 $(git ls-files -- "*.sh" | grep -vE 'src/(secp256k1|univalue)/')

practicalswift · 2018-06-13T15:10:50Z

@MarcoFalke Oh, that's weird! I'm unable to reproduce locally. Will try to find time to investigate soon!

maflcko · 2018-06-13T18:10:48Z

Presumably caused by the non-ascii char in https://github.com/practicalswift/bitcoin/blob/0f556d2e6d19e5fd86712bb5f4b0506924740ae3/test/lint/lint-python.sh#L65

practicalswift · 2018-06-13T21:25:40Z

@MarcoFalke Tried removing that but same issue :-\

laanwj · 2018-06-14T07:08:37Z

Seems like an upstream bug then, with the version installed by Travis, if you can't reproduce it locally?

ken2812221 · 2018-06-14T07:29:00Z

Looks like travis has already installed shellcheck v0.4.6, but we reinstall v0.3.3 from ubuntu repo.

Edit: Tried not to downgrade shellcheck, not going to work.

practicalswift · 2018-06-14T10:07:42Z

Worked around the shellcheck issue by adding an "opt in to locale dependence" mechanism which is now in used for test/lint/lint-shell.sh.

Please re-review :-)

laanwj · 2018-06-14T11:28:58Z

Still segfaults?!?

practicalswift · 2018-06-14T13:24:01Z

@laanwj LC_ALL=C was exported by the lint script runner test/lint/lint-all.sh and thus inherited also by test/lint/lint-shell.sh. Now "opting in to locale dependence" also in lint-all.sh to allow for the linting scripts to opt in/out out of locale dependences themselves :-)

…using "export LC_ALL=C"

laanwj · 2018-06-14T14:16:37Z

Good. We should not forget to remove this workaround when we'll be able to use a newer shellcheck, it's a bit yucky, but checking all scripts but two is much better than nothing.
utACK 47776a9

ken2812221 · 2018-06-16T16:57:08Z

utACK 47776a9

47776a9 Add linter: Make sure all shell scripts opt out of locale dependence using "export LC_ALL=C" (practicalswift) 3352da8 Add "export LC_ALL=C" to all shell scripts (practicalswift) Pull request description: ~~Make sure `LC_ALL=C` is set when using `grep` range expressions.~~ Make sure `LC_ALL=C` is set in all shell scripts. From the `grep(1)` documentation: > Within a bracket expression, a range expression consists of two characters separated by a hyphen. It matches any single character that sorts between the two characters, inclusive, using the locale's collating sequence and character set. For example, in the default C locale, `[a-d]` is equivalent to `[abcd]`. Many locales sort characters in dictionary order, and in these locales `[a-d]` is typically not equivalent to `[abcd]`; it might be equivalent to `[aBbCcDd]`, for example. To obtain the traditional interpretation of bracket expressions, you can use the C locale by setting the `LC_ALL` environment variable to the value C. Context: [Locale issue found when reviewing #13450](https://github.com/bitcoin/bitcoin/pull/13450/files#r194877736) Tree-SHA512: fd74d2612998f9b49ef9be24410e505d8c842716f84d085157fc7f9799d40e8a7b4969de783afcf99b7fae4f91bbb4559651f7dd6578a6a081a50bdea29f0909

maflcko · 2018-06-18T11:24:34Z

@practicalswift This fails travis with

test/lint/lint-all.sh
Missing "export LC_ALL=C" (to avoid locale dependence) as first non-comment non-empty line in test/lint/lint-python-utf8-encoding.sh
^---- failure generated from test/lint/lint-shell-locale.sh

practicalswift · 2018-06-18T12:21:25Z

@MarcoFalke Oh, thanks! Fixed by #13494.

…LL=C 7b23e6e Follow-up to #13454: Fix broken build by exporting LC_ALL=C (practicalswift) Pull request description: Follow-up to #13454: Fix broken build by exporting `LC_ALL=C`. Tree-SHA512: 5cca3182ba034dce28a0df5f4a4b343de6c2526048f17fee30e2f8d946e976b39d9cc54faae6c31bfe89022f9f4c360e9ec8e163a1690bc0656410a48bb81dbf

Follow-up to bitcoin#13454: Fix broken build by exporting LC_ALL=C

47776a9 Add linter: Make sure all shell scripts opt out of locale dependence using "export LC_ALL=C" (practicalswift) 3352da8 Add "export LC_ALL=C" to all shell scripts (practicalswift) Pull request description: ~~Make sure `LC_ALL=C` is set when using `grep` range expressions.~~ Make sure `LC_ALL=C` is set in all shell scripts. From the `grep(1)` documentation: > Within a bracket expression, a range expression consists of two characters separated by a hyphen. It matches any single character that sorts between the two characters, inclusive, using the locale's collating sequence and character set. For example, in the default C locale, `[a-d]` is equivalent to `[abcd]`. Many locales sort characters in dictionary order, and in these locales `[a-d]` is typically not equivalent to `[abcd]`; it might be equivalent to `[aBbCcDd]`, for example. To obtain the traditional interpretation of bracket expressions, you can use the C locale by setting the `LC_ALL` environment variable to the value C. Context: [Locale issue found when reviewing bitcoin#13450](https://github.com/bitcoin/bitcoin/pull/13450/files#r194877736) Tree-SHA512: fd74d2612998f9b49ef9be24410e505d8c842716f84d085157fc7f9799d40e8a7b4969de783afcf99b7fae4f91bbb4559651f7dd6578a6a081a50bdea29f0909

…exporting LC_ALL=C 7b23e6e Follow-up to bitcoin#13454: Fix broken build by exporting LC_ALL=C (practicalswift) Pull request description: Follow-up to bitcoin#13454: Fix broken build by exporting `LC_ALL=C`. Tree-SHA512: 5cca3182ba034dce28a0df5f4a4b343de6c2526048f17fee30e2f8d946e976b39d9cc54faae6c31bfe89022f9f4c360e9ec8e163a1690bc0656410a48bb81dbf

Verifier for scriptable changes Includes changes from the following upstream PRs: - bitcoin/bitcoin#10189 - Excluding the `CNode` scripted changes. - bitcoin/bitcoin#10480 - bitcoin/bitcoin#11390 - bitcoin/bitcoin#13281 - Only the lint scripts we already have. - bitcoin/bitcoin#13454 - Only changes to scripts we already have. - bitcoin/bitcoin#14864 - bitcoin/bitcoin#16327 - Includes some portability fixes to other shell scripts. - bitcoin/bitcoin#20069

47776a9 Add linter: Make sure all shell scripts opt out of locale dependence using "export LC_ALL=C" (practicalswift) 3352da8 Add "export LC_ALL=C" to all shell scripts (practicalswift) Pull request description: ~~Make sure `LC_ALL=C` is set when using `grep` range expressions.~~ Make sure `LC_ALL=C` is set in all shell scripts. From the `grep(1)` documentation: > Within a bracket expression, a range expression consists of two characters separated by a hyphen. It matches any single character that sorts between the two characters, inclusive, using the locale's collating sequence and character set. For example, in the default C locale, `[a-d]` is equivalent to `[abcd]`. Many locales sort characters in dictionary order, and in these locales `[a-d]` is typically not equivalent to `[abcd]`; it might be equivalent to `[aBbCcDd]`, for example. To obtain the traditional interpretation of bracket expressions, you can use the C locale by setting the `LC_ALL` environment variable to the value C. Context: [Locale issue found when reviewing bitcoin#13450](https://github.com/bitcoin/bitcoin/pull/13450/files#r194877736) Tree-SHA512: fd74d2612998f9b49ef9be24410e505d8c842716f84d085157fc7f9799d40e8a7b4969de783afcf99b7fae4f91bbb4559651f7dd6578a6a081a50bdea29f0909

practicalswift force-pushed the avoid-locale-dependent-range-expressions branch from 233a70d to c1a1607 Compare June 13, 2018 06:04

fanquake added the Scripts and tools label Jun 13, 2018

practicalswift force-pushed the avoid-locale-dependent-range-expressions branch from c1a1607 to 0f556d2 Compare June 13, 2018 14:53

maflcko changed the title ~~Add linter: Make sure LC_ALL=C is set when using grep range expressions~~ Make sure LC_ALL=C is set when using grep range expressions Jun 13, 2018

maflcko added the Build system label Jun 13, 2018

practicalswift changed the title ~~Make sure LC_ALL=C is set when using grep range expressions~~ Make sure LC_ALL=C is set in all shell scripts Jun 13, 2018

practicalswift force-pushed the avoid-locale-dependent-range-expressions branch 2 times, most recently from 4286583 to 777212b Compare June 13, 2018 21:16

practicalswift force-pushed the avoid-locale-dependent-range-expressions branch 2 times, most recently from ebe5531 to 6b820ea Compare June 14, 2018 10:06

practicalswift force-pushed the avoid-locale-dependent-range-expressions branch from 6b820ea to 0d02f71 Compare June 14, 2018 13:20

practicalswift added 2 commits June 14, 2018 15:27

Add "export LC_ALL=C" to all shell scripts

3352da8

Add linter: Make sure all shell scripts opt out of locale dependence …

47776a9

…using "export LC_ALL=C"

practicalswift force-pushed the avoid-locale-dependent-range-expressions branch from 0d02f71 to 47776a9 Compare June 14, 2018 13:28

bitcoin deleted a comment from DrahtBot Jun 15, 2018

laanwj merged commit 47776a9 into bitcoin:master Jun 18, 2018

practicalswift added a commit to practicalswift/bitcoin that referenced this pull request Jun 18, 2018

Follow-up to bitcoin#13454: Fix broken build by exporting LC_ALL=C

7b23e6e

practicalswift mentioned this pull request Jun 18, 2018

Follow-up to #13454: Fix broken build by exporting LC_ALL=C #13494

Merged

Bushstar mentioned this pull request Jun 20, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#348

Merged

stamhe pushed a commit to stamhe/bitcoin that referenced this pull request Jun 27, 2018

Follow-up to bitcoin#13454: Fix broken build by exporting LC_ALL=C

925f396

Longyan-Wu pushed a commit to Longyan-Wu/bitcoin that referenced this pull request Jul 1, 2018

Follow-up to bitcoin#13454: Fix broken build by exporting LC_ALL=C

8857da2

SmarterHomes pushed a commit to SmarterHomes/chaincoin that referenced this pull request Sep 11, 2018

Follow-up to bitcoin#13454: Fix broken build by exporting LC_ALL=C

42da5db

SmarterHomes added a commit to chaincoin/chaincoin that referenced this pull request Sep 11, 2018

Merge pull request #437 from HashUnlimited/master

a5e9a06

Follow-up to bitcoin#13454: Fix broken build by exporting LC_ALL=C

This was referenced Oct 26, 2020

Replace BOOST_STATIC_ASSERT with static_assert zcash/zcash#4826

Merged

Verifier for scriptable changes zcash/zcash#4827

Merged

str4d mentioned this pull request Oct 27, 2020

Backport useful lints from upstream zcash/zcash#4829

Merged

practicalswift deleted the avoid-locale-dependent-range-expressions branch April 10, 2021 19:34

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Uh oh!

Conversation

practicalswift commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Jun 13, 2018

Uh oh!

maflcko commented Jun 13, 2018

Uh oh!

practicalswift commented Jun 13, 2018

Uh oh!

maflcko commented Jun 13, 2018

Uh oh!

practicalswift commented Jun 13, 2018

Uh oh!

laanwj commented Jun 14, 2018

Uh oh!

ken2812221 commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Jun 14, 2018

Uh oh!

laanwj commented Jun 14, 2018

Uh oh!

practicalswift commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 14, 2018

Uh oh!

ken2812221 commented Jun 16, 2018

Uh oh!

maflcko commented Jun 18, 2018

Uh oh!

practicalswift commented Jun 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

practicalswift commented Jun 13, 2018 •

edited

Loading

laanwj commented Jun 13, 2018 •

edited

Loading

ken2812221 commented Jun 14, 2018 •

edited

Loading

practicalswift commented Jun 14, 2018 •

edited

Loading