scripts: checkpatch: get codespell dictionary path from package location #4855

ujfalusi · 2021-10-08T08:47:36Z

The standard location of dictionary.txt is under codespell's package, on
my machine atm (codespell 2.1, Artix Linux):
/usr/lib/python3.9/site-packages/codespell_lib/data/dictionary.txt

Since we enable the codespell by default for SOF I have constant:
No codespell typos will be found - file '/usr/share/codespell/dictionary.txt': No such file or directory

The patch proposes to try to fix up the path following the recommendation
found here:
codespell-project/codespell#1540

Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com

marc-hb · 2021-10-08T16:01:12Z

Can you submit this upstream first?

marc-hb · 2021-10-08T16:07:11Z

For some reason this PR breaks... our checkpatch in Github!
In https://github.com/thesofproject/sof/pull/4855/checks?check_run_id=3836565203:

No codespell typos will be found - file '/usr/share/codespell/dictionary.txt': No such file or directory

Whereas in this other, unrelated PR the dictionary is found:
https://github.com/thesofproject/sof/pull/4856/checks?check_run_id=3840142521

WARNING: 'ba' may be misspelled - perhaps 'by'?
Fixes: \82857a78ba04 ("ipc: fix a IPC completion race")

Thanks to its stupidity, we can see codespell ran in that other PR.

marc-hb

not upstream, breaks checkpatch in CI

ujfalusi · 2021-10-08T16:13:05Z

For some reason this PR breaks... our checkpatch in Github! In https://github.com/thesofproject/sof/pull/4855/checks?check_run_id=3836565203:

No codespell typos will be found - file '/usr/share/codespell/dictionary.txt': No such file or directory

Whereas in this other, unrelated PR the dictionary is found: https://github.com/thesofproject/sof/pull/4856/checks?check_run_id=3840142521

WARNING: 'ba' may be misspelled - perhaps 'by'?
Fixes: \82857a78ba04 ("ipc: fix a IPC completion race")

Thanks to its stupidity, we can see codespell ran in that other PR.

I think it complains about the long line in the commit mesaage, which ironically reads:
"No codespell typos will be found - file '/usr/share/codespell/dictionary.txt': No such file or directory"

I'll push a new test commit with short lines to see...

marc-hb · 2021-10-08T16:25:38Z

I think it complains about the long line in the commit mesaage, which ironically reads:
"No codespell typos will be found - file '/usr/share/codespell/dictionary.txt': No such file or directory"

That sounds right (and very funny). Sorry for the confusion. Still: this should be at least submitted (if not merged) upstream first.

Workarounds on my system:

file /usr/share/codespell/dictionary.txt 
/usr/share/codespell/dictionary.txt: symbolic link to //usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt

Also:

$ cat strict_checkpatch.sh
#!/bin/sh

set -ex

./scripts/checkpatch.pl --strict --ignore C99_COMMENT_TOLERANCE \
 --codespell "$@" --emacs --showfile --color=never

ujfalusi · 2021-10-11T06:07:40Z

Changes since v1:

break the quoted error message to avoid long line warning
suppress the STDERR messages (codespell_lib is not found for example) to make the 'experience' better.

ujfalusi · 2021-10-11T06:20:01Z

upstream patch: https://lore.kernel.org/lkml/20211011061839.4199-1-peter.ujfalusi@linux.intel.com/T/#u

ujfalusi · 2021-10-11T06:47:47Z

v2 upstream: https://lore.kernel.org/lkml/20211011063231.6918-1-peter.ujfalusi@linux.intel.com/

submitted upstream

lgirdwood · 2021-10-13T13:19:12Z

CI looks good now, @marc-hb do we need any CI updates prior to merge or are we good to go ?

ujfalusi · 2021-10-13T15:01:48Z

let's wait for my upstream version, it is getting into shape

marc-hb · 2021-10-13T15:24:32Z

@marc-hb do we need any CI updates prior to merge or are we good to go ?

No any change takes effect immediately.

marc-hb · 2021-10-13T15:58:03Z

@marc-hb do we need any CI updates prior to merge or are we good to go ?

Sorry, now I understand your question.

@ujfalusi at your leisure please add to this PR a second commit with this change:

--- a/.github/workflows/codestyle.yml
+++ b/.github/workflows/codestyle.yml
@@ -21,8 +21,7 @@ jobs:
       PR_NUM: ${{github.event.number}}
       # TODO: reduce duplication with scripts/sof-*-commit-hook.sh
       # thanks to either some .conf file or some wrapper script
-      CHK_CMD_OPTS: --ignore UNKNOWN_COMMIT_ID --codespell --codespellfile
-          /usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt
+      CHK_CMD_OPTS: --ignore UNKNOWN_COMMIT_ID --codespell
          --ignore C99_COMMENT_TOLERANCE
     steps:
       # depth 2 so:

This second commit will test your first one.

ujfalusi · 2021-10-14T06:13:12Z

fwiw, v6 upstream: https://lore.kernel.org/lkml/20211014053908.17293-1-peter.ujfalusi@linux.intel.com/

ujfalusi · 2021-10-14T06:15:30Z

@marc-hb do we need any CI updates prior to merge or are we good to go ?

Sorry, now I understand your question.

@ujfalusi at your leisure please add to this PR a second commit with this change:

--- a/.github/workflows/codestyle.yml
+++ b/.github/workflows/codestyle.yml
@@ -21,8 +21,7 @@ jobs:
       PR_NUM: ${{github.event.number}}
       # TODO: reduce duplication with scripts/sof-*-commit-hook.sh
       # thanks to either some .conf file or some wrapper script
-      CHK_CMD_OPTS: --ignore UNKNOWN_COMMIT_ID --codespell --codespellfile
-          /usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt
+      CHK_CMD_OPTS: --ignore UNKNOWN_COMMIT_ID --codespell
          --ignore C99_COMMENT_TOLERANCE
     steps:
       # depth 2 so:

OK, I'll try to remember ;)

This second commit will test your first one.

if the codespell installation is correct it will.

marc-hb · 2021-10-14T06:50:55Z

if the codespell installation is correct it will.

The codespell installation is performed 5 lines below. It's performed with the highly unusual and ground-breaking sudo apt-get -y install codespell

I recommend having a look at the file, it's 55 lines long including whitespace and comments.

ujfalusi · 2021-10-14T07:11:47Z

if the codespell installation is correct it will.

The codespell installation is performed 5 lines below. It's performed with the highly unusual and ground-breaking sudo apt-get -y install codespell

I recommend having a look at the file, it's 55 lines long including whitespace and comments.

Yes, I have checked already.
Not on my machine, but this is a real life example of !installation is correct case:

sudo apt-get -y install codespell
python -c "import os.path as op; import codespell_lib; print(op.join(op.dirname(codespell_lib.__file__), 'data', 'dictionary.txt'))"

Could result in

File "<string>", line 1, in <module>
ImportError: No module named codespell_lib"

all while

# dpkg -L codespell | grep dict
/usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt
/usr/lib/python3/dist-packages/codespell_lib/data/dictionary_code.txt
/usr/lib/python3/dist-packages/codespell_lib/data/dictionary_en-GB_to_en-US.txt
/usr/lib/python3/dist-packages/codespell_lib/data/dictionary_informal.txt
/usr/lib/python3/dist-packages/codespell_lib/data/dictionary_names.txt
/usr/lib/python3/dist-packages/codespell_lib/data/dictionary_rare.txt
/usr/lib/python3/dist-packages/codespell_lib/data/dictionary_usage.txt

Just a heads up.

marc-hb · 2021-10-14T07:22:17Z

Not on my machine, but this is a real life example of ! installation is correct case:

I don't understand. I checked on both Ubuntu and Fedora and in both the python library and the dictionary are part of the same package. This package comes from the single https://github.com/codespell-project/codespell/ project.

Some other Linux distributions like to make their life difficult?

ujfalusi · 2021-10-14T09:33:31Z

Looks like v7 is likely going to make it to mainline:
https://lore.kernel.org/lkml/5455eaf51072c821123ecd458ab810e50b6e8d94.camel@perches.com/

ujfalusi · 2021-10-21T09:35:49Z

Changes since v2:

backported the patch from linux-next
removed the --codespellfile parameter from github flow as it should no longer be needed

marc-hb · 2021-10-21T19:44:05Z

Looks good except for this: https://lore.kernel.org/all/29e25d1364c8ad7f7657cc0660f60c568074d438.camel@perches.com/T/#u

Message-ID <29e25d1364c8ad7f7657cc0660f60c568074d438.camel@perches.com>
not found

ujfalusi · 2021-10-22T05:50:51Z

Looks good except for this: https://lore.kernel.org/all/29e25d1364c8ad7f7657cc0660f60c568074d438.camel@perches.com/T/#u
Message-ID <29e25d1364c8ad7f7657cc0660f60c568074d438.camel@perches.com>
not found

Right, I have notified Andrew Morton and Joe Perches about the wrong link in the applied patch.

mmots:
https://ozlabs.org/~akpm/mmots/broken-out/checkpatch-get-default-codespell-dictionary-path-from-package-location.patch

mmotm:
https://ozlabs.org/~akpm/mmotm/broken-out/checkpatch-get-default-codespell-dictionary-path-from-package-location.patch

linux-next:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/scripts/checkpatch.pl?h=next-20211021&id=54edeb12188f2175bbbc9b26c225b43b3550417d

all contains the same wrong link.

lgirdwood · 2021-10-25T12:24:31Z

@ujfalusi is this PR pending on external fixes now before merge ?

ujfalusi · 2021-10-25T12:27:52Z

@ujfalusi is this PR pending on external fixes now before merge ?

I'm not sure if there will be a fix for the Link: from Andrew Morton to be honest, it is like this in linux-next.

ujfalusi · 2021-10-25T12:28:02Z

@ujfalusi is this PR pending on external fixes now before merge ?

I'm not sure if there will be a fix for the Link: from Andrew Morton to be honest, it is like this in linux-next.

ujfalusi · 2021-10-25T12:29:20Z

Aargh, github UI made me close the PR.

We can wait a day or two, but I don't think there is going to be a fix for it.

lgirdwood · 2021-10-26T20:12:06Z

Aargh, github UI made me close the PR.

We can wait a day or two, but I don't think there is going to be a fix for it.

So IIUC, this is a tmp fix whist this is being fixed upstream. Any reason now not to merge ?

cujomalainey · 2021-10-26T20:42:28Z

we should have something to make sure we revert and reland once fixed upstream so it is not forgotten

ujfalusi · 2021-10-27T04:24:05Z

@lgirdwood, @cujomalainey, I'm fine waiting a bit more but to be honest I don't think that the Link is going to be fixed upstream. I have notified Andrew Morton and Joe Perches on Friday and only got a prompt reply from Joe.

The patch is in linux-next: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/scripts/checkpatch.pl

We can set the deadline for merge to next Monday?

lgirdwood · 2021-10-27T12:15:53Z

@ujfalusi ok, pls remind me on Monday and we can merge if upstream fix is forgotten. I would also recommend a comment as a reminder as @cujomalainey states.

The standard location of dictionary.txt is under codespell's package, on my machine atm (codespell 2.1, Artix Linux): /usr/lib/python3.9/site-packages/codespell_lib/data/dictionary.txt Since we enable the codespell by default for SOF I have constant: No codespell typos will be found - file '/usr/share/codespell/dictionary.txt': No such file or directory The patch proposes to try to fix up the path following the recommendation found here: codespell-project/codespell#1540 Mainline commit: 0ee3e7b8893e ("checkpatch: get default codespell dictionary path from package location") Link: https://lkml.kernel.org/r/29e25d1364c8ad7f7657cc0660f60c568074d438.camel@perches.com Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> Acked-by: Joe Perches <joe@perches.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

With the backported patch to checkpatch.pl the dictionary.txt location should automagically found and no need to specify it by hand. Suggested-by: Marc Herbert <marc.herbert@intel.com> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ujfalusi · 2021-11-10T05:49:21Z

Changes since v3:

Backported the patch from mainline

Note: we have a warning from checkpatch because of unwrapped commit description on the backported patch:

WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#14: 
  No codespell typos will be found - file '/usr/share/codespell/dictionary.txt': No such file or directory

total: 0 errors, 1 warnings, 0 checks, 57 lines checked

ujfalusi · 2021-11-11T08:00:44Z

@marc-hb, @lgirdwood, @cujomalainey, backported the patch from mainline Linux:
0ee3e7b8893e checkpatch: get default codespell dictionary path from package location

ujfalusi requested a review from marc-hb as a code owner October 8, 2021 08:47

ujfalusi requested review from dbaluta, kv2019i, lgirdwood, lyakh, plbossart and ranj063 October 8, 2021 09:06

marc-hb previously requested changes Oct 8, 2021

View reviewed changes

ujfalusi force-pushed the peter/pr/codespell_01 branch from d98bc19 to 267def6 Compare October 11, 2021 06:05

lgirdwood approved these changes Oct 13, 2021

View reviewed changes

ujfalusi marked this pull request as draft October 13, 2021 15:02

ujfalusi force-pushed the peter/pr/codespell_01 branch from 267def6 to 1e03d2f Compare October 21, 2021 09:34

ujfalusi marked this pull request as ready for review October 21, 2021 10:00

ujfalusi requested review from cujomalainey and zrombel as code owners October 21, 2021 10:00

ujfalusi requested a review from marc-hb October 21, 2021 10:00

ujfalusi closed this Oct 25, 2021

ujfalusi reopened this Oct 25, 2021

lgirdwood approved these changes Oct 26, 2021

View reviewed changes

ujfalusi added 2 commits November 10, 2021 07:45

ujfalusi force-pushed the peter/pr/codespell_01 branch from 1e03d2f to d3b4698 Compare November 10, 2021 05:46

lgirdwood approved these changes Nov 11, 2021

View reviewed changes

lgirdwood merged commit aad035a into thesofproject:main Nov 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scripts: checkpatch: get codespell dictionary path from package location #4855

scripts: checkpatch: get codespell dictionary path from package location #4855

ujfalusi commented Oct 8, 2021

marc-hb commented Oct 8, 2021

marc-hb commented Oct 8, 2021

marc-hb left a comment

ujfalusi commented Oct 8, 2021

marc-hb commented Oct 8, 2021 •

edited

Loading

ujfalusi commented Oct 11, 2021

ujfalusi commented Oct 11, 2021

ujfalusi commented Oct 11, 2021

lgirdwood commented Oct 13, 2021

ujfalusi commented Oct 13, 2021

marc-hb commented Oct 13, 2021

marc-hb commented Oct 13, 2021

ujfalusi commented Oct 14, 2021

ujfalusi commented Oct 14, 2021

marc-hb commented Oct 14, 2021

ujfalusi commented Oct 14, 2021

marc-hb commented Oct 14, 2021

ujfalusi commented Oct 14, 2021

ujfalusi commented Oct 21, 2021

marc-hb commented Oct 21, 2021

ujfalusi commented Oct 22, 2021

lgirdwood commented Oct 25, 2021

ujfalusi commented Oct 25, 2021

ujfalusi commented Oct 25, 2021

ujfalusi commented Oct 25, 2021

lgirdwood commented Oct 26, 2021

cujomalainey commented Oct 26, 2021

ujfalusi commented Oct 27, 2021

lgirdwood commented Oct 27, 2021

ujfalusi commented Nov 10, 2021

ujfalusi commented Nov 11, 2021

scripts: checkpatch: get codespell dictionary path from package location #4855

scripts: checkpatch: get codespell dictionary path from package location #4855

Conversation

ujfalusi commented Oct 8, 2021

marc-hb commented Oct 8, 2021

marc-hb commented Oct 8, 2021

marc-hb left a comment

Choose a reason for hiding this comment

ujfalusi commented Oct 8, 2021

marc-hb commented Oct 8, 2021 • edited Loading

ujfalusi commented Oct 11, 2021

ujfalusi commented Oct 11, 2021

ujfalusi commented Oct 11, 2021

lgirdwood commented Oct 13, 2021

ujfalusi commented Oct 13, 2021

marc-hb commented Oct 13, 2021

marc-hb commented Oct 13, 2021

ujfalusi commented Oct 14, 2021

ujfalusi commented Oct 14, 2021

marc-hb commented Oct 14, 2021

ujfalusi commented Oct 14, 2021

marc-hb commented Oct 14, 2021

ujfalusi commented Oct 14, 2021

ujfalusi commented Oct 21, 2021

marc-hb commented Oct 21, 2021

ujfalusi commented Oct 22, 2021

lgirdwood commented Oct 25, 2021

ujfalusi commented Oct 25, 2021

ujfalusi commented Oct 25, 2021

ujfalusi commented Oct 25, 2021

lgirdwood commented Oct 26, 2021

cujomalainey commented Oct 26, 2021

ujfalusi commented Oct 27, 2021

lgirdwood commented Oct 27, 2021

ujfalusi commented Nov 10, 2021

ujfalusi commented Nov 11, 2021

marc-hb commented Oct 8, 2021 •

edited

Loading