[PATCH v2 00/16] Range of copy-firmware/check_whence fixes

[PATCH v2 00/16] Range of copy-firmware/check_whence fixes

[Emil Velikov via B4 Relay]

Hello all,

With the latest python 3.12 check_whence.py started throwing some
warnings. The more I looked around, the more loose ends and bugs reared
their (ugly) heads.

In this series:
 - check_whence.py: sort the filenames, use consistent variable names
 - check_whence.py: fix links-to-links and band them
 - editorconfig: add the config and fix the yaml+sh indentation
 - copy-firmware.sh: factor out dedup fix broken ad-hoc symlink handling
 - copy-firmware.sh: remove the hacky argument injection
 - copy-firmware.sh: warn if the destination folder is NOT empty
 - copy-firmware.sh: call ./check_whence.py and remove the dead code

The series is a mix of cosmetics (style, names), existing bug fixes and
trying loose ends. I'm happy to split it in sub-series, as per your
suggestions.

As always, input is highly appreciated. Thanks o/

To: linux-firmware@kernel.org

Changes in v2:
- Commit message tweaks
- Correct a few flipped test checks
- Rename deduplicate-firmware.sh to dedup-firmware.sh
- Change line width (editorconfig) to 90
- Properly handle destdir
- Link to v1: https://lore.kernel.org/r/20240922-misc-fixes-v1-0-8c6e52997c94@gmail.com

---
Emil Velikov (16):
      check_whence.py: use consistent naming
      check_whence.py: ban link-to-a-link
      check_whence.py: LC_ALL=C sort -u the filelist
      check_whence.py: annotate replacement strings as raw
      editorconfig: add initial config file
      Style update yaml files
      copy-firmware.sh: flesh out and fix dedup-firmware.sh
      Revert "copy-firmware: Support additional compressor options"
      copy-firmware.sh: reset and consistently handle destdir
      copy-firmware.sh: fix indentation
      copy-firmware.sh: add err() helper
      copy-firmware.sh: warn if the destination folder is not empty
      copy-firmware.sh: call ./check_whence.py before parsing the file
      copy-firmware.sh: remove no longer reachable test -f
      copy-firmware.sh: remove no longer reachable test -L
      copy-firmware.sh: rename variables in symlink hanlding

 .editorconfig           |  21 +++++++++
 .gitlab-ci.yml          |   2 +-
 .pre-commit-config.yaml |  38 ++++++++--------
 Makefile                |   9 ++--
 WHENCE                  |  14 +++---
 check_whence.py         |  42 +++++++++++-------
 copy-firmware.sh        | 113 +++++++++++++-----------------------------------
 dedup-firmware.sh       |  52 ++++++++++++++++++++++
 8 files changed, 162 insertions(+), 129 deletions(-)
---
base-commit: 6c88d9b8253b8ec6df701a551a56438ea2e5bacf
change-id: 20240922-misc-fixes-8a13acc71a93

Best regards,
-- 
Emil Velikov <emil.l.velikov@gmail.com>

[PATCH v2 01/16] check_whence.py: use consistent naming

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

Namely: file(name) and sym(link) ... which were swapped in a few places.
One of which by yours truly :facepalm:

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 check_whence.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/check_whence.py b/check_whence.py
index fd74a56a5c5a1fbbc989588626ba1d9bc072d602..c270ee6020083044645353235a94eee0b2497960 100755
--- a/check_whence.py
+++ b/check_whence.py
@@ -99,7 +99,7 @@ def main():
         sys.stderr.write("E: %s listed in WHENCE as File, but is directory\n" % name)
         ret = 1
 
-    for name in set(fw for fw in whence_files if whence_files.count(fw) > 1):
+    for name in set(name for name in whence_files if whence_files.count(name) > 1):
         sys.stderr.write("E: %s listed in WHENCE twice\n" % name)
         ret = 1
 
@@ -107,7 +107,7 @@ def main():
         sys.stderr.write("E: %s listed in WHENCE twice\n" % name)
         ret = 1
 
-    for name in set(link for link in whence_files if os.path.islink(link)):
+    for name in set(file for file in whence_files if os.path.islink(file)):
         sys.stderr.write("E: %s listed in WHENCE as File, but is a symlink\n" % name)
         ret = 1
 
@@ -131,10 +131,10 @@ def main():
                 break
             valid_targets.add(dirname)
 
-    for name, target in sorted(links_list):
+    for link, target in sorted(links_list):
         if target not in valid_targets:
             sys.stderr.write(
-                "E: target %s of link %s in WHENCE" " does not exist\n" % (target, name)
+                "E: target %s of link %s in WHENCE" " does not exist\n" % (target, link)
             )
             ret = 1
 

-- 
2.46.1

[PATCH v2 02/16] check_whence.py: ban link-to-a-link

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

Using link-to-a-link reduces legibility and changes to the root link,
also changes the leafs. Where the latter may be undesired and in some
cases just wrong.

We have a couple of instances in-tree, so fix them up and ban the
combination.

One particularly good example, why we'd want this is:
https://gitlab.com/kernel-firmware/linux-firmware/-/merge_requests/272

In there we have the following:

    File: ti/tas2781/TAS2XXX1EB30.bin
    [snip]
    Link: TAS2XXX1EB3.bin -> ti/tas2781/TAS2XXX1EB3.bin
    Link: ti/tas2781/TAS2XXX1EB3.bin -> TAS2XXX1EB30.bin
    Link: TAS2XXX1EB30.bin -> ti/tas2781/TAS2XXX1EB30.bin

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 WHENCE          | 14 +++++++-------
 check_whence.py | 12 ++++++++++--
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/WHENCE b/WHENCE
index 3897121a115e185cc36e782b33ef66e43d87d557..6af1a97e2672922f2a3a1fcf0306b158eae1b6f7 100644
--- a/WHENCE
+++ b/WHENCE
@@ -3686,7 +3686,7 @@ File: ath10k/WCN3990/hw1.0/qcm2290/wlanmdsp.mbn
 Version: WLAN.HL.3.3.7.c2-00931-QCAHLSWMTPLZ-1
 Link: ath10k/WCN3990/hw1.0/qrb4210/wlanmdsp.mbn -> ../qcm2290/wlanmdsp.mbn
 Link: qcom/qcm2290/wlanmdsp.mbn -> ../../ath10k/WCN3990/hw1.0/qcm2290/wlanmdsp.mbn
-Link: qcom/qrb4210/wlanmdsp.mbn -> ../../ath10k/WCN3990/hw1.0/qrb4210/wlanmdsp.mbn
+Link: qcom/qrb4210/wlanmdsp.mbn -> ../../ath10k/WCN3990/hw1.0/qcm2290/wlanmdsp.mbn
 File: ath10k/WCN3990/hw1.0/qcm2290/firmware-5.bin
 Link: ath10k/WCN3990/hw1.0/qrb4210/firmware-5.bin -> ../qcm2290/firmware-5.bin
 
@@ -5214,11 +5214,11 @@ Link: nvidia/gp104/acr/bl.bin -> ../../gp102/acr/bl.bin
 Link: nvidia/gp104/acr/ucode_load.bin -> ../../gp102/acr/ucode_load.bin
 Link: nvidia/gp104/acr/ucode_unload.bin -> ../../gp102/acr/ucode_unload.bin
 Link: nvidia/gp104/acr/unload_bl.bin -> ../../gp102/acr/unload_bl.bin
-Link: nvidia/gp104/gr/fecs_bl.bin -> ../../gp102/gr/fecs_bl.bin
+Link: nvidia/gp104/gr/fecs_bl.bin -> ../../gm200/gr/fecs_bl.bin
 File: nvidia/gp104/gr/fecs_data.bin
 File: nvidia/gp104/gr/fecs_inst.bin
 File: nvidia/gp104/gr/fecs_sig.bin
-Link: nvidia/gp104/gr/gpccs_bl.bin -> ../../gp102/gr/gpccs_bl.bin
+Link: nvidia/gp104/gr/gpccs_bl.bin -> ../../gm200/gr/gpccs_bl.bin
 File: nvidia/gp104/gr/gpccs_data.bin
 File: nvidia/gp104/gr/gpccs_inst.bin
 File: nvidia/gp104/gr/gpccs_sig.bin
@@ -5237,11 +5237,11 @@ Link: nvidia/gp106/acr/bl.bin -> ../../gp102/acr/bl.bin
 Link: nvidia/gp106/acr/ucode_load.bin -> ../../gp102/acr/ucode_load.bin
 Link: nvidia/gp106/acr/ucode_unload.bin -> ../../gp102/acr/ucode_unload.bin
 Link: nvidia/gp106/acr/unload_bl.bin -> ../../gp102/acr/unload_bl.bin
-Link: nvidia/gp106/gr/fecs_bl.bin -> ../../gp102/gr/fecs_bl.bin
+Link: nvidia/gp106/gr/fecs_bl.bin -> ../../gm200/gr/fecs_bl.bin
 File: nvidia/gp106/gr/fecs_data.bin
 Link: nvidia/gp106/gr/fecs_inst.bin -> ../../gp102/gr/fecs_inst.bin
 File: nvidia/gp106/gr/fecs_sig.bin
-Link: nvidia/gp106/gr/gpccs_bl.bin -> ../../gp102/gr/gpccs_bl.bin
+Link: nvidia/gp106/gr/gpccs_bl.bin -> ../../gm200/gr/gpccs_bl.bin
 File: nvidia/gp106/gr/gpccs_data.bin
 Link: nvidia/gp106/gr/gpccs_inst.bin -> ../../gp102/gr/gpccs_inst.bin
 File: nvidia/gp106/gr/gpccs_sig.bin
@@ -5924,10 +5924,10 @@ File: netronome/flower/nic_AMDA0096.nffw
 File: netronome/flower/nic_AMDA0097.nffw
 File: netronome/flower/nic_AMDA0058.nffw
 Link: netronome/flower/nic_AMDA0081.nffw -> nic_AMDA0097.nffw
-Link: netronome/flower/nic_AMDA0081-0001_1x40.nffw -> nic_AMDA0081.nffw
+Link: netronome/flower/nic_AMDA0081-0001_1x40.nffw -> nic_AMDA0097.nffw
 Link: netronome/flower/nic_AMDA0097-0001_2x40.nffw -> nic_AMDA0097.nffw
 Link: netronome/flower/nic_AMDA0099-0001_2x10.nffw -> nic_AMDA0099.nffw
-Link: netronome/flower/nic_AMDA0081-0001_4x10.nffw -> nic_AMDA0081.nffw
+Link: netronome/flower/nic_AMDA0081-0001_4x10.nffw -> nic_AMDA0097.nffw
 Link: netronome/flower/nic_AMDA0097-0001_4x10_1x40.nffw -> nic_AMDA0097.nffw
 Link: netronome/flower/nic_AMDA0099-0001_2x25.nffw -> nic_AMDA0099.nffw
 Link: netronome/flower/nic_AMDA0096-0001_2x10.nffw -> nic_AMDA0096.nffw
diff --git a/check_whence.py b/check_whence.py
index c270ee6020083044645353235a94eee0b2497960..d7742f1ed951acab6efc89916c7f6621ba56881b 100755
--- a/check_whence.py
+++ b/check_whence.py
@@ -115,12 +115,20 @@ def main():
         sys.stderr.write("E: %s listed in WHENCE as Link, is in tree\n" % name)
         ret = 1
 
+    invalid_targets = set(link[0] for link in links_list)
+    for link, target in sorted(links_list):
+        if target in invalid_targets:
+            sys.stderr.write(
+                "E: target %s of link %s is also a link\n" % (target, link)
+            )
+            ret = 1
+
     for name in sorted(list(known_files - git_files)):
         sys.stderr.write("E: %s listed in WHENCE does not exist\n" % name)
         ret = 1
 
-    # A link can point to another link, or to a file...
-    valid_targets = set(link[0] for link in links_list) | git_files
+    # A link can point to a file...
+    valid_targets = set(git_files)
 
     # ... or to a directory
     for target in set(valid_targets):

-- 
2.46.1

[PATCH v2 03/16] check_whence.py: LC_ALL=C sort -u the filelist

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

Sort the file list, so it's easier to manage.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 check_whence.py | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/check_whence.py b/check_whence.py
index d7742f1ed951acab6efc89916c7f6621ba56881b..dbbb68f7ab51b8db52c1d2334a6d4d20962bbef8 100755
--- a/check_whence.py
+++ b/check_whence.py
@@ -73,23 +73,23 @@ def main():
     whence_links = list(zip(*links_list))[0]
     known_files = set(name for name in whence_list if not name.endswith("/")) | set(
         [
-            ".gitignore",
             ".codespell.cfg",
+            ".gitignore",
             ".gitlab-ci.yml",
             ".pre-commit-config.yaml",
-            "build_packages.py",
-            "check_whence.py",
-            "configure",
+            "Dockerfile",
             "Makefile",
             "README.md",
-            "copy-firmware.sh",
             "WHENCE",
-            "Dockerfile",
+            "build_packages.py",
+            "check_whence.py",
+            "configure",
+            "contrib/process_linux_firmware.py",
             "contrib/templates/debian.changelog",
             "contrib/templates/debian.control",
             "contrib/templates/debian.copyright",
             "contrib/templates/rpm.spec",
-            "contrib/process_linux_firmware.py",
+            "copy-firmware.sh",
         ]
     )
     known_prefixes = set(name for name in whence_list if name.endswith("/"))

-- 
2.46.1

[PATCH v2 04/16] check_whence.py: annotate replacement strings as raw

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

Otherwise python 3.12 throws warnings like below:

.../check_whence.py:40: SyntaxWarning: invalid escape sequence '\ '
  yield match.group(1).replace("\ ", " ").replace('"', "")

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 check_whence.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/check_whence.py b/check_whence.py
index dbbb68f7ab51b8db52c1d2334a6d4d20962bbef8..64dc133551a68aeb24dbf7f02f617ea3913477b2 100755
--- a/check_whence.py
+++ b/check_whence.py
@@ -37,7 +37,7 @@ def list_whence_files():
         for line in whence:
             match = re.match(r"(?:RawFile|File):\s*(.*)", line)
             if match:
-                yield match.group(1).replace("\ ", " ").replace('"', "")
+                yield match.group(1).replace(r"\ ", " ").replace('"', "")
                 continue
 
 
@@ -48,8 +48,8 @@ def list_links_list():
             if match:
                 linkname, target = match.group(1).split("->")
 
-                linkname = linkname.strip().replace("\ ", " ").replace('"', "")
-                target = target.strip().replace("\ ", " ").replace('"', "")
+                linkname = linkname.strip().replace(r"\ ", " ").replace('"', "")
+                target = target.strip().replace(r"\ ", " ").replace('"', "")
 
                 # Link target is relative to the link
                 target = os.path.join(os.path.dirname(linkname), target)

-- 
2.46.1

[PATCH v2 05/16] editorconfig: add initial config file

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

It's a simple format handled by practically every supported platform or program
out there.

Add an initial configuration file, so we reduce the style variation of the files
in-tree.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 .editorconfig   | 21 +++++++++++++++++++++
 check_whence.py |  1 +
 2 files changed, 22 insertions(+)

diff --git a/.editorconfig b/.editorconfig
new file mode 100644
index 0000000000000000000000000000000000000000..100bbcebaac4514cb2b8377c1e0b98f0c6964d9b
--- /dev/null
+++ b/.editorconfig
@@ -0,0 +1,21 @@
+# To use this config on you editor, follow the instructions at:
+# http://editorconfig.org
+
+root = true
+
+[*]
+charset = utf-8
+insert_final_newline = true
+tab_width = 8
+max_line_length = 90
+
+[Makefile]
+indent_style = tab
+
+[*.{sh,py}]
+indent_style = space
+indent_size = 4
+
+[*.{yml,yaml}]
+indent_style = space
+indent_size = 2
diff --git a/check_whence.py b/check_whence.py
index 64dc133551a68aeb24dbf7f02f617ea3913477b2..afe97a76cf5ed609117d07387a19c2092fa00c6c 100755
--- a/check_whence.py
+++ b/check_whence.py
@@ -74,6 +74,7 @@ def main():
     known_files = set(name for name in whence_list if not name.endswith("/")) | set(
         [
             ".codespell.cfg",
+            ".editorconfig",
             ".gitignore",
             ".gitlab-ci.yml",
             ".pre-commit-config.yaml",

-- 
2.46.1

[PATCH v2 06/16] Style update yaml files

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 .gitlab-ci.yml          |  2 +-
 .pre-commit-config.yaml | 38 +++++++++++++++++++-------------------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index ae96f7d859fc34ebc74c93a93993f623e05d78a2..e05ef3bb81c8f15049a3b068daac9fff35184d17 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -11,7 +11,7 @@ check-commits:
   stage: test
   image: registry.gitlab.com/kernel-firmware/linux-firmware
   rules:
-   - if: $CI_MERGE_REQUEST_ID
+    - if: $CI_MERGE_REQUEST_ID
   script:
     - ci-fairy check-commits --signed-off-by --textwidth=0
 
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 7a49d063250e755b531059fe24b5a571d311005f..370a2970616bda3a340ebe523cd2ee2bb57759c1 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -1,30 +1,30 @@
 default_stages: [commit]
 repos:
--   repo: https://github.com/pre-commit/pre-commit-hooks
+  - repo: https://github.com/pre-commit/pre-commit-hooks
     rev: v4.0.1
     hooks:
-    -   id: check-executables-have-shebangs
-    -   id: forbid-new-submodules
-    -   id: check-yaml
-    -   id: check-symlinks
-    -   id: destroyed-symlinks
--   repo: https://github.com/shellcheck-py/shellcheck-py
+      - id: check-executables-have-shebangs
+      - id: forbid-new-submodules
+      - id: check-yaml
+      - id: check-symlinks
+      - id: destroyed-symlinks
+  - repo: https://github.com/shellcheck-py/shellcheck-py
     rev: v0.9.0.5
     hooks:
-    -   id: shellcheck
--   repo: https://github.com/ambv/black
+      - id: shellcheck
+  - repo: https://github.com/ambv/black
     rev: 22.6.0
     hooks:
-    - id: black
--   repo: https://github.com/igorshubovych/markdownlint-cli
+      - id: black
+  - repo: https://github.com/igorshubovych/markdownlint-cli
     rev: v0.33.0
     hooks:
-    - id: markdownlint
-      args: ['--fix']
--   repo: local
+      - id: markdownlint
+        args: ['--fix']
+  - repo: local
     hooks:
-    - id: check-whence
-      name: Check whence
-      files: 'WHENCE'
-      language: script
-      entry: ./check_whence.py
+      - id: check-whence
+        name: Check whence
+        files: 'WHENCE'
+        language: script
+        entry: ./check_whence.py

-- 
2.46.1

[PATCH v2 07/16] copy-firmware.sh: flesh out and fix dedup-firmware.sh

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

Flesh out the de-duplication logic in separate script. The copy-firmware.sh is
already complex enough and de-duplication doesn't really fit in there.

In the process we migrate away from the open-coded `ln --relative`. We also
avoid touching symlinks, which are not created by rdfind. Otherwise we end up
"fixing" the folder to folder symlinks (created earlier in the process) and
things explode.

As result we also get a few bonuses:
 - the COPYOPTS shell injection is gone - the variable was never used
 - people can dedup as separate step if/when they choose to do so

Aside: based on the noise in git log and around distros ... I'm wondering if
having the de-duplication as opt-in, would have been better. Is it too late to
change or the ship has sailed?

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 Makefile          |  9 +++++----
 check_whence.py   |  1 +
 copy-firmware.sh  | 23 -----------------------
 dedup-firmware.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/Makefile b/Makefile
index ac43c97fbd0a35b610c4d21c2bf8e8cf21864c9a..216dd4885b5bdf35189bbcca325fc7535acbe8f0 100644
--- a/Makefile
+++ b/Makefile
@@ -26,21 +26,22 @@ deb:
 rpm:
 	./build_packages.py --rpm
 
-install:
-	install -d $(DESTDIR)$(FIRMWAREDIR)
-	./copy-firmware.sh $(COPYOPTS) $(DESTDIR)$(FIRMWAREDIR)
+install: install-nodedup
+	./dedup-firmware.sh $(DESTDIR)$(FIRMWAREDIR)
 
 install-nodedup:
 	install -d $(DESTDIR)$(FIRMWAREDIR)
-	./copy-firmware.sh --ignore-duplicates $(DESTDIR)$(FIRMWAREDIR)
+	./copy-firmware.sh $(DESTDIR)$(FIRMWAREDIR)
 
 install-xz:
 	install -d $(DESTDIR)$(FIRMWAREDIR)
 	./copy-firmware.sh --xz $(DESTDIR)$(FIRMWAREDIR)
+	./dedup-firmware.sh $(DESTDIR)$(FIRMWAREDIR)
 
 install-zst:
 	install -d $(DESTDIR)$(FIRMWAREDIR)
 	./copy-firmware.sh --zstd $(DESTDIR)$(FIRMWAREDIR)
+	./dedup-firmware.sh $(DESTDIR)$(FIRMWAREDIR)
 
 clean:
 	rm -rf release dist
diff --git a/check_whence.py b/check_whence.py
index afe97a76cf5ed609117d07387a19c2092fa00c6c..09dbdd5439e738385c0402e244bad11ed9fc5558 100755
--- a/check_whence.py
+++ b/check_whence.py
@@ -91,6 +91,7 @@ def main():
             "contrib/templates/debian.copyright",
             "contrib/templates/rpm.spec",
             "copy-firmware.sh",
+            "dedup-firmware.sh",
         ]
     )
     known_prefixes = set(name for name in whence_list if name.endswith("/"))
diff --git a/copy-firmware.sh b/copy-firmware.sh
index 6c557f234dd2b90e68ba26429ad3e788f2201c4f..344f478db74a7080b34c264645d6bcd9a80384a8 100755
--- a/copy-firmware.sh
+++ b/copy-firmware.sh
@@ -9,7 +9,6 @@ prune=no
 # shellcheck disable=SC2209
 compress=cat
 compext=
-skip_dedup=0
 
 while test $# -gt 0; do
     case $1 in
@@ -45,11 +44,6 @@ while test $# -gt 0; do
             shift
             ;;
 
-        --ignore-duplicates)
-            skip_dedup=1
-            shift
-            ;;
-
         -*)
             if test "$compress" = "cat"; then
                 echo "ERROR: unknown command-line option: $1"
@@ -75,13 +69,6 @@ if [ -z "$destdir" ]; then
 	exit 1
 fi
 
-if ! command -v rdfind >/dev/null; then
-	if [ "$skip_dedup" != 1 ]; then
-    		echo "ERROR: rdfind is not installed.  Pass --ignore-duplicates to skip deduplication"
-		exit 1
-	fi
-fi
-
 # shellcheck disable=SC2162 # file/folder name can include escaped symbols
 grep -E '^(RawFile|File):' WHENCE | sed -E -e 's/^(RawFile|File): */\1 /;s/"//g' | while read k f; do
     test -f "$f" || continue
@@ -95,16 +82,6 @@ grep -E '^(RawFile|File):' WHENCE | sed -E -e 's/^(RawFile|File): */\1 /;s/"//g'
     fi
 done
 
-if [ "$skip_dedup" != 1 ] ; then
-	$verbose "Finding duplicate files"
-	rdfind -makesymlinks true -makeresultsfile false "$destdir" >/dev/null
-	find "$destdir" -type l | while read -r l; do
-		target="$(realpath "$l")"
-		$verbose "Correcting path for $l"
-		ln -fs "$(realpath --relative-to="$(dirname "$(realpath -s "$l")")" "$target")" "$l"
-	done
-fi
-
 # shellcheck disable=SC2162 # file/folder name can include escaped symbols
 grep -E '^Link:' WHENCE | sed -e 's/^Link: *//g;s/-> //g' | while read f d; do
     if test -L "$f$compext"; then
diff --git a/dedup-firmware.sh b/dedup-firmware.sh
new file mode 100755
index 0000000000000000000000000000000000000000..2bbd637f0736252027bfe1f60f886772efec1d08
--- /dev/null
+++ b/dedup-firmware.sh
@@ -0,0 +1,52 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# Deduplicate files in a given destdir
+#
+
+err() {
+    echo "ERROR: $*"
+    exit 1
+}
+
+verbose=:
+destdir=
+while test $# -gt 0; do
+    case $1 in
+        -v | --verbose)
+            # shellcheck disable=SC2209
+            verbose=echo
+            ;;
+        *)
+            if test -n "$destdir"; then
+                err "unknown command-line options: $*"
+            fi
+
+            destdir="$1"
+            shift
+            ;;
+    esac
+done
+
+if test -z "$destdir"; then
+    err "destination directory was not specified."
+fi
+
+if ! test -d "$destdir"; then
+    err "provided directory does not exit."
+fi
+
+if ! command -v rdfind >/dev/null; then
+    err "rdfind is not installed."
+fi
+
+$verbose "Finding duplicate files"
+rdfind -makesymlinks true -makeresultsfile true "$destdir" >/dev/null
+
+grep DUPTYPE_WITHIN_SAME_TREE results.txt | grep -o "$destdir.*" | while read -r l; do
+    target="$(realpath "$l")"
+    $verbose "Correcting path for $l"
+    ln --force --symbolic --relative "$target" "$l"
+done
+
+rm results.txt

-- 
2.46.1

[PATCH v2 08/16] Revert "copy-firmware: Support additional compressor options"

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

This reverts commit 2bad80e7edd3c0f84718545d9338c8edfed16513.

The commit effectively added accidental command injection, while it was aiming
to control the compression flags.

In practise you'd want to use ZSTD_CLEVEL and ZSTD_NBTHREADS for zstd. As
documented in zstd(1) it allows for up-to level 19, which is fine since the
kernel does not support higher levels.

Arch, Alpine and likely other distributions have been using this approach
since day one.

The other compressors like xz have equivalent.

Cc: Juerg Haefliger <juerg.haefliger@canonical.com>
Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 copy-firmware.sh | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/copy-firmware.sh b/copy-firmware.sh
index 344f478db74a7080b34c264645d6bcd9a80384a8..21ed20d5886fe71588b923914265f992cf5c29b0 100755
--- a/copy-firmware.sh
+++ b/copy-firmware.sh
@@ -44,14 +44,6 @@ while test $# -gt 0; do
             shift
             ;;
 
-        -*)
-            if test "$compress" = "cat"; then
-                echo "ERROR: unknown command-line option: $1"
-                exit 1
-            fi
-            compress="$compress $1"
-            shift
-            ;;
         *)
             if test "x$destdir" != "x"; then
                 echo "ERROR: unknown command-line options: $*"

-- 
2.46.1

[PATCH v2 09/16] copy-firmware.sh: reset and consistently handle destdir

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

Currently we don't reset/override the destdir variable, so we end up inheriting
whatever the caller's environment has for it. While it may work, it's not
particularly consistent (be that within the script or other tools) nor is it
obvious.

While in here, ensure we handle the variable with test -z/-n instead of varying
other constructs.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 copy-firmware.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/copy-firmware.sh b/copy-firmware.sh
index 21ed20d5886fe71588b923914265f992cf5c29b0..65a1b39b93293d1f876ea0cd2991e811247e9ecd 100755
--- a/copy-firmware.sh
+++ b/copy-firmware.sh
@@ -9,6 +9,7 @@ prune=no
 # shellcheck disable=SC2209
 compress=cat
 compext=
+destdir=
 
 while test $# -gt 0; do
     case $1 in
@@ -45,7 +46,7 @@ while test $# -gt 0; do
             ;;
 
         *)
-            if test "x$destdir" != "x"; then
+            if test -n "$destdir"; then
                 echo "ERROR: unknown command-line options: $*"
                 exit 1
             fi
@@ -56,7 +57,7 @@ while test $# -gt 0; do
     esac
 done
 
-if [ -z "$destdir" ]; then
+if test -z "$destdir"; then
 	echo "ERROR: destination directory was not specified"
 	exit 1
 fi

-- 
2.46.1

[PATCH v2 10/16] copy-firmware.sh: fix indentation

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 copy-firmware.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/copy-firmware.sh b/copy-firmware.sh
index 65a1b39b93293d1f876ea0cd2991e811247e9ecd..f83b4775cd2ce873b0f6fa76c4d61df3bcf1b5df 100755
--- a/copy-firmware.sh
+++ b/copy-firmware.sh
@@ -58,8 +58,8 @@ while test $# -gt 0; do
 done
 
 if test -z "$destdir"; then
-	echo "ERROR: destination directory was not specified"
-	exit 1
+    echo "ERROR: destination directory was not specified"
+    exit 1
 fi
 
 # shellcheck disable=SC2162 # file/folder name can include escaped symbols
@@ -115,9 +115,9 @@ done
 
 # Verify no broken symlinks
 if test "$(find "$destdir" -xtype l | wc -l)" -ne 0 ; then
-        echo "ERROR: Broken symlinks found:"
-        find "$destdir" -xtype l
-        exit 1
+    echo "ERROR: Broken symlinks found:"
+    find "$destdir" -xtype l
+    exit 1
 fi
 
 exit 0

-- 
2.46.1

[PATCH v2 11/16] copy-firmware.sh: add err() helper

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 copy-firmware.sh | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/copy-firmware.sh b/copy-firmware.sh
index f83b4775cd2ce873b0f6fa76c4d61df3bcf1b5df..610c8b65b2b3bdb9f723da3656112ac121e51c84 100755
--- a/copy-firmware.sh
+++ b/copy-firmware.sh
@@ -11,6 +11,11 @@ compress=cat
 compext=
 destdir=
 
+err() {
+    echo -e "ERROR: $*"
+    exit 1
+}
+
 while test $# -gt 0; do
     case $1 in
         -v | --verbose)
@@ -26,8 +31,7 @@ while test $# -gt 0; do
 
         --xz)
             if test "$compext" = ".zst"; then
-                echo "ERROR: cannot mix XZ and ZSTD compression"
-                exit 1
+                err "cannot mix XZ and ZSTD compression"
             fi
             compress="xz --compress --quiet --stdout --check=crc32"
             compext=".xz"
@@ -36,8 +40,7 @@ while test $# -gt 0; do
 
         --zstd)
             if test "$compext" = ".xz"; then
-                echo "ERROR: cannot mix XZ and ZSTD compression"
-                exit 1
+                err "cannot mix XZ and ZSTD compression"
             fi
             # shellcheck disable=SC2209
             compress="zstd --compress --quiet --stdout"
@@ -47,8 +50,7 @@ while test $# -gt 0; do
 
         *)
             if test -n "$destdir"; then
-                echo "ERROR: unknown command-line options: $*"
-                exit 1
+                err "unknown command-line options: $*"
             fi
 
             destdir="$1"
@@ -58,8 +60,7 @@ while test $# -gt 0; do
 done
 
 if test -z "$destdir"; then
-    echo "ERROR: destination directory was not specified"
-    exit 1
+    err "destination directory was not specified"
 fi
 
 # shellcheck disable=SC2162 # file/folder name can include escaped symbols
@@ -115,9 +116,7 @@ done
 
 # Verify no broken symlinks
 if test "$(find "$destdir" -xtype l | wc -l)" -ne 0 ; then
-    echo "ERROR: Broken symlinks found:"
-    find "$destdir" -xtype l
-    exit 1
+    err "Broken symlinks found:\n$(find "$destdir" -xtype l)"
 fi
 
 exit 0

-- 
2.46.1

[PATCH v2 12/16] copy-firmware.sh: warn if the destination folder is not empty

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

If the user provides an existing non-empty folder (their /usr/lib/firmware/ or otherwise)
there is a high chance we'll silently overwrite existing files. That may or may not be what
they wanted, so throw a warning so highlight that.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 copy-firmware.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/copy-firmware.sh b/copy-firmware.sh
index 610c8b65b2b3bdb9f723da3656112ac121e51c84..12b97ae32f953c96a1c7612ccd14b1991a8687bf 100755
--- a/copy-firmware.sh
+++ b/copy-firmware.sh
@@ -16,6 +16,10 @@ err() {
     exit 1
 }
 
+warn() {
+    echo "WARNING: $*"
+}
+
 while test $# -gt 0; do
     case $1 in
         -v | --verbose)
@@ -63,6 +67,10 @@ if test -z "$destdir"; then
     err "destination directory was not specified"
 fi
 
+if test -d "$destdir"; then
+    find "$destdir" -type d -empty >/dev/null || warn "destination folder is not empty."
+fi
+
 # shellcheck disable=SC2162 # file/folder name can include escaped symbols
 grep -E '^(RawFile|File):' WHENCE | sed -E -e 's/^(RawFile|File): */\1 /;s/"//g' | while read k f; do
     test -f "$f" || continue

-- 
2.46.1

[PATCH v2 13/16] copy-firmware.sh: call ./check_whence.py before parsing the file

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

Currently ./check_whence.py is used when submitting new firmware, while
copy-firmware.sh when the firmware is to be consumed.

Since the latter does (very little) validation, having a malformed WHENCE file
can lead to all sorted of problems. From the obvious, where it errors out, to
more serious one where it overwrites or executes something it should not have.

Just call check_whence.py and error out. It takes 0.2s on my 5 year old
mid-range laptop, so the overhead is negligible.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 copy-firmware.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/copy-firmware.sh b/copy-firmware.sh
index 12b97ae32f953c96a1c7612ccd14b1991a8687bf..12322530e5e155f77829f0d070d173ab5bfbab5b 100755
--- a/copy-firmware.sh
+++ b/copy-firmware.sh
@@ -71,6 +71,9 @@ if test -d "$destdir"; then
     find "$destdir" -type d -empty >/dev/null || warn "destination folder is not empty."
 fi
 
+$verbose "Checking that WHENCE file is formatted properly"
+./check_whence.py || err "check_whence.py has detected errors."
+
 # shellcheck disable=SC2162 # file/folder name can include escaped symbols
 grep -E '^(RawFile|File):' WHENCE | sed -E -e 's/^(RawFile|File): */\1 /;s/"//g' | while read k f; do
     test -f "$f" || continue

-- 
2.46.1

[PATCH v2 14/16] copy-firmware.sh: remove no longer reachable test -f

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

With previous commit we call check_whence.py, which ensures that all files
listed are available. Drop the now dead code.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 copy-firmware.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/copy-firmware.sh b/copy-firmware.sh
index 12322530e5e155f77829f0d070d173ab5bfbab5b..bdc20d596ef5f9d99b17e9cea74a0358f0f4de09 100755
--- a/copy-firmware.sh
+++ b/copy-firmware.sh
@@ -76,7 +76,6 @@ $verbose "Checking that WHENCE file is formatted properly"
 
 # shellcheck disable=SC2162 # file/folder name can include escaped symbols
 grep -E '^(RawFile|File):' WHENCE | sed -E -e 's/^(RawFile|File): */\1 /;s/"//g' | while read k f; do
-    test -f "$f" || continue
     install -d "$destdir/$(dirname "$f")"
     $verbose "copying/compressing file $f$compext"
     if test "$compress" != "cat" && test "$k" = "RawFile"; then

-- 
2.46.1

[PATCH v2 15/16] copy-firmware.sh: remove no longer reachable test -L

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

The check_whence.py script ensures that links defined in WHENCE are not in-tree.
Since we're calling the script, we no longer need the convoluted path and
associated --prune tag.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 copy-firmware.sh | 46 ++++++++--------------------------------------
 1 file changed, 8 insertions(+), 38 deletions(-)

diff --git a/copy-firmware.sh b/copy-firmware.sh
index bdc20d596ef5f9d99b17e9cea74a0358f0f4de09..57d43cd004231e34c5349ccff90072b26af63093 100755
--- a/copy-firmware.sh
+++ b/copy-firmware.sh
@@ -5,7 +5,6 @@
 #
 
 verbose=:
-prune=no
 # shellcheck disable=SC2209
 compress=cat
 compext=
@@ -28,11 +27,6 @@ while test $# -gt 0; do
             shift
             ;;
 
-        -P | --prune)
-            prune=yes
-            shift
-            ;;
-
         --xz)
             if test "$compext" = ".zst"; then
                 err "cannot mix XZ and ZSTD compression"
@@ -88,39 +82,15 @@ done
 
 # shellcheck disable=SC2162 # file/folder name can include escaped symbols
 grep -E '^Link:' WHENCE | sed -e 's/^Link: *//g;s/-> //g' | while read f d; do
-    if test -L "$f$compext"; then
-        test -f "$destdir/$f$compext" && continue
-        $verbose "copying link $f$compext"
-        install -d "$destdir/$(dirname "$f")"
-        cp -d "$f$compext" "$destdir/$f$compext"
-
-        if test "x$d" != "x"; then
-            target="$(readlink "$f")"
-
-            if test "x$target" != "x$d"; then
-                $verbose "WARNING: inconsistent symlink target: $target != $d"
-            else
-                if test "x$prune" != "xyes"; then
-                    $verbose "WARNING: unneeded symlink detected: $f"
-                else
-                    $verbose "WARNING: pruning unneeded symlink $f"
-                    rm -f "$f$compext"
-                fi
-            fi
-        else
-            $verbose "WARNING: missing target for symlink $f"
-        fi
+    directory="$destdir/$(dirname "$f")"
+    install -d "$directory"
+    target="$(cd "$directory" && realpath -m -s "$d")"
+    if test -e "$target"; then
+        $verbose "creating link $f -> $d"
+        ln -s "$d" "$destdir/$f"
     else
-        directory="$destdir/$(dirname "$f")"
-        install -d "$directory"
-        target="$(cd "$directory" && realpath -m -s "$d")"
-        if test -e "$target"; then
-            $verbose "creating link $f -> $d"
-            ln -s "$d" "$destdir/$f"
-        else
-            $verbose "creating link $f$compext -> $d$compext"
-            ln -s "$d$compext" "$destdir/$f$compext"
-        fi
+        $verbose "creating link $f$compext -> $d$compext"
+        ln -s "$d$compext" "$destdir/$f$compext"
     fi
 done
 

-- 
2.46.1

[PATCH v2 16/16] copy-firmware.sh: rename variables in symlink hanlding

[Emil Velikov via B4 Relay]

From: Emil Velikov <emil.l.velikov@gmail.com>

Currently we use f(ile) and d(irectory), over the more common ones t(arget) and
l(ink). Rename things appropriately.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 copy-firmware.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/copy-firmware.sh b/copy-firmware.sh
index 57d43cd004231e34c5349ccff90072b26af63093..e2d2945c926872e55800c2a637574e8234908a5d 100755
--- a/copy-firmware.sh
+++ b/copy-firmware.sh
@@ -81,16 +81,16 @@ grep -E '^(RawFile|File):' WHENCE | sed -E -e 's/^(RawFile|File): */\1 /;s/"//g'
 done
 
 # shellcheck disable=SC2162 # file/folder name can include escaped symbols
-grep -E '^Link:' WHENCE | sed -e 's/^Link: *//g;s/-> //g' | while read f d; do
-    directory="$destdir/$(dirname "$f")"
+grep -E '^Link:' WHENCE | sed -e 's/^Link: *//g;s/-> //g' | while read l t; do
+    directory="$destdir/$(dirname "$l")"
     install -d "$directory"
-    target="$(cd "$directory" && realpath -m -s "$d")"
+    target="$(cd "$directory" && realpath -m -s "$t")"
     if test -e "$target"; then
-        $verbose "creating link $f -> $d"
-        ln -s "$d" "$destdir/$f"
+        $verbose "creating link $l -> $t"
+        ln -s "$t" "$destdir/$l"
     else
-        $verbose "creating link $f$compext -> $d$compext"
-        ln -s "$d$compext" "$destdir/$f$compext"
+        $verbose "creating link $l$compext -> $t$compext"
+        ln -s "$t$compext" "$destdir/$l$compext"
     fi
 done
 

-- 
2.46.1

Re: [PATCH v2 00/16] Range of copy-firmware/check_whence fixes

[Mario Limonciello]

On 9/23/24 08:09, Emil Velikov via B4 Relay wrote:
> Hello all,
> 
> With the latest python 3.12 check_whence.py started throwing some
> warnings. The more I looked around, the more loose ends and bugs reared
> their (ugly) heads.
> 
> In this series:
>   - check_whence.py: sort the filenames, use consistent variable names
>   - check_whence.py: fix links-to-links and band them
>   - editorconfig: add the config and fix the yaml+sh indentation
>   - copy-firmware.sh: factor out dedup fix broken ad-hoc symlink handling
>   - copy-firmware.sh: remove the hacky argument injection
>   - copy-firmware.sh: warn if the destination folder is NOT empty
>   - copy-firmware.sh: call ./check_whence.py and remove the dead code
> 
> The series is a mix of cosmetics (style, names), existing bug fixes and
> trying loose ends. I'm happy to split it in sub-series, as per your
> suggestions.
> 
> As always, input is highly appreciated. Thanks o/
> 
> To: linux-firmware@kernel.org

Hi,

I passed this into a MR in Gitlab and it fails CI.

https://gitlab.com/kernel-firmware/linux-firmware/-/merge_requests/316

Can you please run `make check` before submitting again?

Also ideally please submit a MR instead of an email.  Then you can force 
push until CI passes or take into account any feedback.

Thanks!
> 
> Changes in v2:
> - Commit message tweaks
> - Correct a few flipped test checks
> - Rename deduplicate-firmware.sh to dedup-firmware.sh
> - Change line width (editorconfig) to 90
> - Properly handle destdir
> - Link to v1: https://lore.kernel.org/r/20240922-misc-fixes-v1-0-8c6e52997c94@gmail.com
> 
> ---
> Emil Velikov (16):
>        check_whence.py: use consistent naming
>        check_whence.py: ban link-to-a-link
>        check_whence.py: LC_ALL=C sort -u the filelist
>        check_whence.py: annotate replacement strings as raw
>        editorconfig: add initial config file
>        Style update yaml files
>        copy-firmware.sh: flesh out and fix dedup-firmware.sh
>        Revert "copy-firmware: Support additional compressor options"
>        copy-firmware.sh: reset and consistently handle destdir
>        copy-firmware.sh: fix indentation
>        copy-firmware.sh: add err() helper
>        copy-firmware.sh: warn if the destination folder is not empty
>        copy-firmware.sh: call ./check_whence.py before parsing the file
>        copy-firmware.sh: remove no longer reachable test -f
>        copy-firmware.sh: remove no longer reachable test -L
>        copy-firmware.sh: rename variables in symlink hanlding
> 
>   .editorconfig           |  21 +++++++++
>   .gitlab-ci.yml          |   2 +-
>   .pre-commit-config.yaml |  38 ++++++++--------
>   Makefile                |   9 ++--
>   WHENCE                  |  14 +++---
>   check_whence.py         |  42 +++++++++++-------
>   copy-firmware.sh        | 113 +++++++++++++-----------------------------------
>   dedup-firmware.sh       |  52 ++++++++++++++++++++++
>   8 files changed, 162 insertions(+), 129 deletions(-)
> ---
> base-commit: 6c88d9b8253b8ec6df701a551a56438ea2e5bacf
> change-id: 20240922-misc-fixes-8a13acc71a93
> 
> Best regards,