Introduce a setting to disable Spotlight while running the test suite
ClosedPublic

Authored by aprantl on Mar 9 2018, 5:17 PM.

Details

Summary

This is a more principled approach to disabling Spotlight .dSYM lookups while running the testsuite, most importantly it also works for the LIT-based tests, which I overlooked in my initial fix (renaming the test build dir to lldb-tests.noindex).

Diff Detail

Repository
rL LLVM
aprantl created this revision.Mar 9 2018, 5:17 PM

Does that mean we can remove the .noindex thingy from the build folder name? (In any case, the change seems fine to me).

I would rather see this done in a more generic fashion. Any platform should be able to enable/disable external symbol lookups. Maybe make this setting:

(lldb) settings set symbols.enable-external-lookups false

On Darwin this would mean dSYM. Any other platform can skip any symbol lookups that aren't part of the object file or specified inside the object file.

Does that mean we can remove the .noindex thingy from the build folder name? (In any case, the change seems fine to me).

Yes, but we could still keep it for performance reasons. Perhaps only on Darwin.

aprantl updated this revision to Diff 138082.Mar 12 2018, 12:56 PM

Renamed option to something more general.

Thanks for the suggestion, Greg! I renamed to option to use a more generic name and removed the #if APPLE guards.

clayborg added inline comments.Mar 12 2018, 1:00 PM
source/Core/ModuleList.cpp
70 ↗(On Diff #138082)

Remove this comment? I can apply to any platform in the future that implements symbol lookups right?

73–74 ↗(On Diff #138082)

Maybe restate this a bit? Maybe something like:

"Disable using external tools or linked libraries to locate symbol files."
aprantl updated this revision to Diff 138086.Mar 12 2018, 1:05 PM

More review feedback.

aprantl marked 2 inline comments as done.Mar 12 2018, 1:05 PM
vsk accepted this revision.Mar 12 2018, 1:40 PM

Thanks @aprantl!

This revision is now accepted and ready to land.Mar 12 2018, 1:40 PM
This revision was automatically updated to reflect the committed changes.

This is great. I think I'm supposed to complain that there isn't a positive test that this flag works. This is in the testsuite, but that will just give a negative signal - that we aren't getting random failures that are maybe due to leaving built test products around on your indexed volumes somewhere.

If we weren't building in a .noindex hierarchy this would be as simple as building the dSYM variant, then moving the dSYM to a subdirectory and making sure we still don't read in the dSYM. But that won't work in a .noindex build. Not sure we should be copying files to people's desktops. /tmp/ won't work because Spotlight doesn't index there. We talked briefly about having an indexed directory for test products as well as a no index one, which you could then use for that purpose.

But I'm not sure that this switch is worth that effort. This is fine by me as is.