[PATCH] D112759: [llvm] [Debuginfo] Add llvm-debuginfod-find tool and end-to-end-tests.

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: llvm/test/tools/llvm-debuginfod-find/debuginfod-find.py:2-3
+# REQUIRES: curl
+# RUN: rm -rf %t && mkdir %t && python %S/debuginfod-find.py %S/Inputs \
+# RUN: llvm-debuginfod-find %t
+import threading
----------------
you don't have to chain the commands with &&. The script is automatically aborted when any command fails.

This would be much clearer if you just put every command on a separate line instead of peppering it with `&&` and `\`'s.


================
Comment at: llvm/test/tools/llvm-debuginfod-find/debuginfod-find.py:36
+            if process.wait() != 0:
+                print('process returned nontrivial error code')
+                return 1
----------------
It may be helpful to print what it was.


================
Comment at: llvm/test/tools/llvm-debuginfod-find/debuginfod-find.py:38
+                return 1
+            fname = os.path.join(cache_directory,
+                f'llvmcache-{xxhash}')
----------------
I am somewhat surprised by the need to peek inside the cache directory. What is the actual interface of the tool, or how is the user expected to know where to locate the file that it has downloaded? It seems like the elfutils debuginfod-find prints the path to the file on stdout. Should this do the same?

(I mean, it may be useful to hardcode the cache path so you can verify the file has been downloaded to the right place, but i don't think that every user should need to know the precise hash algorithm just to be able to locate the downloaded file.)


================
Comment at: llvm/test/tools/llvm-debuginfod-find/debuginfod-find.py:41
+            if not os.path.exists(fname):
+                print(f'file {fname} doesnt exist')
+                return 1
----------------
or even "does not", for maximum formality.


================
Comment at: llvm/test/tools/llvm-debuginfod-find/debuginfod-find.py:45-46
+                if infile.read() != contents:
+                    print('file contents not as expected')
+                    return 1
+
----------------
Instead of doing the checks here, it may be better to just print the relevant data (exit code, file contents, whatever) and then verify that through an external FileCheck command.

The reason I'm proposing this is to improve the error message in case this fails. Like, imagine this test fails on a bot somewhere. "file contents does not match" will not tell you much, as it does not even say which file it refers to. OTOH, with FileCheck you get the "i expected <this> but got <that>" functionality for free. (You can print `repr(file.read())` and use `FileCheck --strict-whitespace`, if you want to catch any issues with newlines or unprintable characters.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112759/new/

https://reviews.llvm.org/D112759