Page MenuHomePhabricator
Differential D4183

Added support for `export LC_ALL=C.UTF-8` to the shell linter
ClosedPublic
Actions

Authored by jasonbcox on Sep 30 2019, 23:04.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Project
Commits
rSTAGING7af048cfc2b0: Added support for `export LC_ALL=C.UTF-8` to the shell linter
rABC7af048cfc2b0: Added support for `export LC_ALL=C.UTF-8` to the shell linter
Summary

Similar in spirit to https://github.com/bitcoin/bitcoin/pull/13863/commits/414326952cd6086ebc491413e8cf58a60ad857a4#diff-41e4d53455228facb9cfa198baaa2440

This allows us to explicitly set the codeset to UTF-8.

Test Plan

arc lint on the following cases:

  • script with export LC_ALL=C
  • script with no LC_ALL set. Fails as expected.
  • script with export LC_ALL=C.UTF-8
  • script with export LC_ALL=C.UTF-9 Fails as expected (sanity check that we don't accidentally match on export LC_ALL=C)

Diff Detail

Repository
rABC Bitcoin ABC
Branch
lc-all
Lint
Lint Errors
SeverityLocationCodeMessage
Errorarcanist/linter/ShellLocaleLinter.php:1PHPCS.E.Generic.PHP.RequireStrictTypes.MissingDeclarationGeneric.PHP.RequireStrictTypes.MissingDeclaration
Unit
No Test Coverage
Build Status
Buildable 7638
Build 13316: Bitcoin ABC Buildbot (legacy)
Build 13315: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Sep 30 2019, 23:04
Herald added a reviewer: deadalnix. · View Herald TranscriptSep 30 2019, 23:04
teamcity added a comment.Sep 30 2019, 23:04

Build Bitcoin-ABC / Diffs / Diff Testing started.

jasonbcox added a child revision: D4184: Change CI builds to use utf-8 encoding.Sep 30 2019, 23:06
teamcity added a comment.Sep 30 2019, 23:13

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7638: Diff 13279.Sep 30 2019, 23:13
Fabien requested changes to this revision.Oct 1 2019, 06:31
Fabien added a subscriber: Fabien.
Fabien added inline comments.
arcanist/linter/ShellLocaleLinter.php
1

You should have a very recent phpcs because this rule is 3 months old according to github:
https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/Generic/Sniffs/PHP/RequireStrictTypesSniff.php

It enforces using the new PHP 7 strict_types declaration: https://www.php.net/manual/fr/control-structures.declare.php.

Because none of our code nor arcanist code is compliant, an exception can be added to the rule file (arcanist/phpcs.xml).

61

PHP has the array_search() built-in for doing this:

if (array_search(trim($matches[0][0]), self::LOCALE_STATEMENTS, true) !== false) {
  return $this->raiseLintAtPath(
      self::INVALID_LOCALE,
      pht('Shell scripts should set the locale to avoid side effects.')
    );
}
This revision now requires changes to proceed.Oct 1 2019, 06:31
Fabien added a comment.Oct 1 2019, 06:33

Maybe there should be an explicit notice of backport here, as the remaining parts of the PR are Travis related ?

jasonbcox marked an inline comment as done.Oct 1 2019, 17:56
jasonbcox added inline comments.
arcanist/linter/ShellLocaleLinter.php
1

D4193

jasonbcox marked 2 inline comments as done.Oct 1 2019, 18:05
jasonbcox added inline comments.
arcanist/linter/ShellLocaleLinter.php
61

cool. equality is flipped though ;)

jasonbcox updated this revision to Diff 13299.Oct 1 2019, 18:10
jasonbcox marked an inline comment as done.

Rebase + Fix according to feedback

teamcity added a comment.Oct 1 2019, 18:10

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B7653: Diff 13299.Oct 1 2019, 18:22
teamcity added a comment.Oct 1 2019, 18:22

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Fabien accepted this revision.Oct 1 2019, 18:47
Fabien added inline comments.
arcanist/linter/ShellLocaleLinter.php
61

Oops... good catch !

This revision is now accepted and ready to land.Oct 1 2019, 18:47
Closed by commit rABC7af048cfc2b0: Added support for `export LC_ALL=C.UTF-8` to the shell linter (authored by jasonbcox). · Explain WhyOct 1 2019, 18:55
This revision was automatically updated to reflect the committed changes.
jasonbcox added a commit: rABC7af048cfc2b0: Added support for `export LC_ALL=C.UTF-8` to the shell linter.
jasonbcox added a commit: rSTAGING7af048cfc2b0: Added support for `export LC_ALL=C.UTF-8` to the shell linter.Oct 9 2019, 20:10

Revision Contents
Changeset List

PathSize
arcanist/
linter/
27 lines

Diff 13279

View Options

arcanist/linter/ShellLocaleLinter.php

Loading...