Add Travis test to make sure text files are UTF-8 encoded #14701

donatieng · 2021-05-25T16:54:00Z

Summary of changes

Introducing text files not encoded with UTF-8 breaks our docs CI.
This PR cleans up the relevant file and adds a check to our CI to prevent issues.

Impact of changes

Migration actions required

Documentation

Nonee

Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR

Reviewers

@ARMmbed/mbed-os-maintainers

ciarmcom · 2021-05-25T17:00:18Z

@donatieng, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

donatieng · 2021-05-25T20:01:20Z

CI job test: https://travis-ci.com/github/ARMmbed/mbed-os/jobs/507867979

LDong-Arm

@donatieng Thanks for the fix!

Patater · 2021-05-26T09:47:06Z

connectivity/drivers/ble/FEATURE_BLE/TARGET_STM32WB/STM32Cube_FW/shci.h

@@ -502,7 +502,7 @@ extern "C" {
   * MaxConnEventLength
   * This parameter determines the maximum duration of a slave connection event. When this duration is reached the slave closes
   * the current connections event (whatever is the CE_length parameter specified by the master in HCI_CREATE_CONNECTION HCI command),
-   * expressed in units of 625/256 �s (~2.44 �s)
+   * expressed in units of 625/256 us (~2.44 us)


µ is UTF-8. The codepoint is U+03BC, which is encoded in UTF-8 as: 0xCE 0xBC (https://www.compart.com/en/unicode/U+03BC)

Good point. I think we should agree on what characters are allowed in Mbed OS. ASCII feels a bit restrictive but is more compatible. If we allow any UTF-8, then the CircleCI needs to be fixed instead.

This particular character was added a few weeks ago, so our code base was likely ASCII-only before that.

What breaks exactly? What tool chokes on this? I didn't see it explained in the commit message.

The title says "don't allow invalid UTF-8". Does this mean if encoded correctly (in a valid manner), µ would be OK?

Confusingly, the description says "Introducing non-UTF-8 characters in text files breaks our docs CI.", but then removes a UTF-8 character. I'm not sure what is breaking what. Maybe we need to correctly encode shci.h as UTF-8 rather than remove the µ?

The doc_builder tool breaks when it tries to do a pathlib.Path.read_text. If the file it's reading from is saved with utf8 encoding, then µ should work ok.

>>> import pathlib >>> import sys >>> sys.getdefaultencoding() 'utf-8' >>> pathlib.Path("test.txt").read_text() 'μ\n'

Ah so the real issue is the encoding of that file rather than the presence of µ?

So, rather than replacing µ, we should convert the file to utf-8. For example, in vim you can do https://stackoverflow.com/a/66572180. This keeps the greek letter.

Yup the issue is that the file is not encoded as UTF-8:

file -I shci.h shci.h: text/x-c; charset=unknown-8bit

Happy to try and convert the file to keep the "µ"s.

Patater · 2021-05-26T09:48:07Z

.travis.yml

+    - <<: *basic-vm
+      name: "UTF-8 Check"
+      script:
+        # Make sure we're not introducing any non-UTF-8 character in text files


Do you mean "non-ASCII" character?

A possible utf-8 check is

file -I <filename>

and the output should contain charset=utf-8.

Ah see above 😄 ⬆️

Pull request has been modified.

Patater · 2021-05-26T13:01:20Z

.travis.yml

+    - <<: *basic-vm
+      name: "UTF-8 Check"
+      script:
+        # Make sure we're not introducing any non-UTF-8 character in text files


This comment doesn't make sense. Pretty much any character is going to be encodable by UTF-8.

I can rephrase - meant invalid encoding

Patater · 2021-05-26T13:02:32Z

.travis.yml

+      name: "UTF-8 Check"
+      script:
+        # Make sure we're not introducing any non-UTF-8 character in text files
+        - git diff $TRAVIS_BRANCH...HEAD -U0 | ( grep -a '^+' || true ) | ( ! grep -axv '.*' )


Is this check still valid or should we run file -I <filename> and check the encoding is one of the handful of acceptable options (at least ascii, and utf-8)

From what I've read, file -I doesn't check the whole file and is therefore not always correct

@Patater hopefully 15c9474 addresses your comment

LDong-Arm

@donatieng
Please squash the commits to one for shci.h and one for adding Travis test. And update the title & PR description to text encoding instead of character.

LGTM otherwise, thanks for identifying and fixing the issue! I'll unblock a number of docs PRs.

donatieng · 2021-06-02T11:15:29Z

Thanks @LDong-Arm!

Squashed and updated 🙂

LDong-Arm

LGTM

LDong-Arm · 2021-06-02T11:39:17Z

@ARMmbed/mbed-os-maintainers Shall we run CI and merge this, as it unblocks PRs in https://github.com/ARMmbed/mbed-os-5-docs/pulls, thanks

0xc0170 · 2021-06-02T11:57:42Z

connectivity/drivers/ble/FEATURE_BLE/TARGET_STM32WB/STM32Cube_FW/shci.h

@@ -435,7 +435,7 @@ extern "C" {
   * PrWriteListSize
   * NOTE: This parameter is ignored by the CPU2 when the parameter "Options" is set to "LL_only" ( see Options description in that structure )
   *
-   * Maximum number of supported �prepare write request�
+   * Maximum number of supported prepare write request


@ARMmbed/team-st-mcd Please forward this change upstream

0xc0170 · 2021-06-02T11:58:23Z

CI started

mbed-ci · 2021-06-02T12:17:37Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label May 25, 2021

ciarmcom requested a review from a team May 25, 2021 17:00

ciarmcom added the needs: review label May 25, 2021

mergify bot added needs: work and removed needs: review labels May 25, 2021

donatieng force-pushed the utf-8-check branch 7 times, most recently from 27ab346 to 9f20cc9 Compare May 25, 2021 20:00

donatieng marked this pull request as ready for review May 25, 2021 20:01

LDong-Arm approved these changes May 26, 2021

View reviewed changes

Patater previously requested changes May 26, 2021

View reviewed changes

Patater reviewed May 26, 2021

View reviewed changes

LDong-Arm suggested changes Jun 2, 2021

View reviewed changes

donatieng changed the title ~~Add Travis test to detect any invalid UTF-8 character~~ Add Travis test to make sure text files are UTF-8 encoded Jun 2, 2021

Donatien Garnier added 2 commits June 2, 2021 12:12

Convert shci.h from (presumably) latin1 encoding to UTF-8

decd7d0

Add Travis test to detect any non-UTF-8-encoded text

eac5956

donatieng force-pushed the utf-8-check branch from 15c9474 to eac5956 Compare June 2, 2021 11:14

LDong-Arm approved these changes Jun 2, 2021

View reviewed changes

0xc0170 approved these changes Jun 2, 2021

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Jun 2, 2021

mergify bot added ready for merge and removed needs: CI labels Jun 2, 2021

0xc0170 merged commit be9850b into ARMmbed:master Jun 2, 2021

mergify bot removed the ready for merge label Jun 2, 2021

mbedmain added release-version: 6.12.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jun 18, 2021

Add Travis test to make sure text files are UTF-8 encoded #14701

Add Travis test to make sure text files are UTF-8 encoded #14701

Uh oh!

Conversation

donatieng commented May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented May 25, 2021

Uh oh!

donatieng commented May 25, 2021

Uh oh!

LDong-Arm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Patater May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwalton-arm May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LDong-Arm May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donatieng May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LDong-Arm left a comment

Choose a reason for hiding this comment

Uh oh!

donatieng commented Jun 2, 2021

Uh oh!

LDong-Arm left a comment

Choose a reason for hiding this comment

Uh oh!

LDong-Arm commented Jun 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Jun 2, 2021

Uh oh!

mbed-ci commented Jun 2, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

Uh oh!

Uh oh!

donatieng commented May 25, 2021 •

edited

Loading

Patater May 26, 2021 •

edited

Loading

rwalton-arm May 26, 2021 •

edited

Loading

LDong-Arm May 26, 2021 •

edited

Loading

donatieng May 26, 2021 •

edited

Loading