Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure LC_ALL=C is set in all shell scripts #13454

Merged

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jun 13, 2018

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

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

laanwj commented Jun 13, 2018

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 practicalswift force-pushed the avoid-locale-dependent-range-expressions branch from c1a1607 to 0f556d2 Compare June 13, 2018 14:53
@maflcko 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
@practicalswift
Copy link
Contributor Author

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

@practicalswift 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
@maflcko
Copy link
Member

maflcko commented Jun 13, 2018

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
Copy link
Contributor Author

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

@maflcko
Copy link
Member

maflcko commented Jun 13, 2018

@practicalswift 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
Copy link
Contributor Author

@MarcoFalke Tried removing that but same issue :-\

@laanwj
Copy link
Member

laanwj commented Jun 14, 2018

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

@ken2812221
Copy link
Contributor

ken2812221 commented Jun 14, 2018

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 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
Copy link
Contributor Author

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
Copy link
Member

laanwj commented Jun 14, 2018

Still segfaults?!?

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

practicalswift commented Jun 14, 2018

@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 :-)

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

laanwj commented Jun 14, 2018

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

@bitcoin bitcoin deleted a comment from DrahtBot Jun 15, 2018
@ken2812221
Copy link
Contributor

utACK 47776a9

@laanwj laanwj merged commit 47776a9 into bitcoin:master Jun 18, 2018
laanwj added a commit that referenced this pull request Jun 18, 2018
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
Copy link
Member

maflcko commented Jun 18, 2018

@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
Copy link
Contributor Author

@MarcoFalke Oh, thanks! Fixed by #13494.

maflcko pushed a commit that referenced this pull request Jun 18, 2018
…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
stamhe pushed a commit to stamhe/bitcoin that referenced this pull request Jun 27, 2018
Longyan-Wu pushed a commit to Longyan-Wu/bitcoin that referenced this pull request Jul 1, 2018
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Sep 11, 2018
HashUnlimited added a commit to chaincoin/chaincoin that referenced this pull request Sep 11, 2018
Follow-up to bitcoin#13454: Fix broken build by exporting LC_ALL=C
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 29, 2020
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 29, 2020
…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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 29, 2020
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 29, 2020
…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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 29, 2020
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 29, 2020
…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
zkbot added a commit to zcash/zcash that referenced this pull request Oct 27, 2020
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
@practicalswift practicalswift deleted the avoid-locale-dependent-range-expressions branch April 10, 2021 19:34
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Mar 8, 2022
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants