[PATCH] D113717: [Symbolizer][Debuginfo] Add debuginfod client to llvm-symbolizer.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D113717#3162690 <https://reviews.llvm.org/D113717#3162690>, @noajshu wrote:

> Thank you for the helpful comments @jhenderson , I just uploaded a change to address them.
>
> In D113717#3159258 <https://reviews.llvm.org/D113717#3159258>, @dblaikie wrote:
>
>> In D113717#3157634 <https://reviews.llvm.org/D113717#3157634>, @jhenderson wrote:
>>
>>> Not looked at anything else in the patch stack, but I can more-or-less guess what is going on (storing the debug data on a remote server for the symbolizer and other tools to be able to look up?). If so, in general, this looks fine to me. My only slight concern is I wonder how many places (especially build bots) will exercise this test, since it requires a new dependency. Might be worth approaching some build bot owners and asking if they'd be willing to install curl as part of their setup.
>>
>> I seem to recall some mention that this API was usable with a local cache even without the curl dependency? Perhaps a test using only a local cache (not needing a server or curl) could be added as well? (the test with curl/etc might be good to keep too - though perhaps it could be removed if the API functionality is already tested elsewhere - if it isn't/can't readily be tested elsewhere then here's fine too)
>
> D112758 <https://reviews.llvm.org/D112758> includes unit testing that verifies the local cache works without curl. I updated the end-to-end tests of this diff (D113717 <https://reviews.llvm.org/D113717>) and added an additional test that just hits the local cache and does not require Curl. I also added checks for the Curl-dependent test to verify that even after the server is taken down, the debuginfo is still accessible in the local cache.
> Is it ok to keep all of these tests? If I had to pick between the end-to-end cache test here and the unit test in D112758 <https://reviews.llvm.org/D112758>, I would favor the unit test since it's more likely to be run on all bots. However the end-to-end test here is additionally checking that the debuginfod client has been properly integrated with the symbolizer.

I'd probably be inclined to skip the testing that requires this standalone python server - and only test llvm-symbolizer via the non-curl-dependent local cache. The API surface area seems small enough that it's pretty unlikely that the unit tests for networked cases could pass, local cache testing of llvm-symbolizer could pass, but then networked llvm-symbolizer functionality would still be broken?

Oh... but the unit testing only tests the local cache - so this llvm-symbolizer test and python server is the only testing of the remote part? It'd be nice if there is lower level and higher level testing, that the lower level testing should be more comprehensive - if it'd be feasible/reasonable to test the API directly with networking functionality?

> I think it's a great idea to make sure libcurl will be tested on some buildbots. Since `LLVM_ENABLE_CURL` defaults to `ON` in D112753 <https://reviews.llvm.org/D112753>, llvm will be built with curl by default as long as it's found by cmake. It's possible the library is already installed on some buildbots. Do you know how to view the buildbot configuration scripts?

Not sure, really :/ the buildbots are a bit haphazard.



================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.py:1
+# This script is used to test the debuginfod client functionality of the symbolizer.
+# It first stands up a Python HTTP static file server and then executes the client.
----------------
Any file that isn't itself a test file (with RUN lines, etc) even if it isn't currently found by the lit config as a test, should go in an "Inputs" subdirectory.


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

https://reviews.llvm.org/D113717