Page MenuHomePhabricator

[lldb] Make the majority of the lit configuration values optional for the API tests
ClosedPublic

Authored by JDevlieghere on Aug 28 2020, 3:20 PM.

Details

Summary

LIT uses a model where the test suite is configurable trough a lit.site.cfg file. Most of the time we use the lit.site.cfg which has a bunch of default values, generated by CMake, that match the current build configuration. However, nothing prevents you from running the test suite with a different configuration, either by overriding some of these values from the command line, or by passing a different lit.site.cfg.

Many of those configurationvalues are optional but they still need to be set because lit.cfg.py is accessing them directly. This patch changes the code to use getattr to return the attribute if it exists. This makes it possible to specify a minimal lit.site.cfg with only the mandatory/desired configuration values.

Diff Detail

Event Timeline

JDevlieghere created this revision.Aug 28 2020, 3:20 PM
JDevlieghere requested review of this revision.Aug 28 2020, 3:20 PM
rupprecht accepted this revision.Aug 28 2020, 4:29 PM
This revision is now accepted and ready to land.Aug 28 2020, 4:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2020, 6:08 PM
labath added inline comments.Sep 2 2020, 7:46 AM
lldb/test/API/lit.cfg.py
88–110

I don't think this name and the implementation are a good match. Given that the usages are a matching the name and not the implementation, maybe just change the implementation to hasattr(config, attr)?

That said, I'm not sure that is_configured("foo") is significantly better than hasattr(config, "foo"), so it might be better to just delete the function altogether.

JDevlieghere marked an inline comment as done.Sep 2 2020, 9:38 AM
JDevlieghere added inline comments.
lldb/test/API/lit.cfg.py
88–110

It does more than just hasattr(config, "foo"), is does hasattr(config, "foo") and config.foo. The option can exist but be set to False or None.

JDevlieghere marked an inline comment as done.Sep 2 2020, 9:42 AM
JDevlieghere added inline comments.
lldb/test/API/lit.cfg.py
88–110

I couldn't think of a good Python way of turning this into a bool instead of relying not the caller to do so, maybe you do?

labath added inline comments.Sep 3 2020, 12:06 AM
lldb/test/API/lit.cfg.py
88–110

It does more than just hasattr(config, "foo"), is does hasattr(config, "foo") and config.foo.

Ah, right. I did miss that distinction.

I couldn't think of a good Python way of turning this into a bool instead of relying not the caller to do so, maybe you do?

Well, there's always the option to say if hasattr(config, "foo") and config.foo: return True. However, it's not clear to me why this needs to handle so many cases. I can sort of see how treating a missing attribute and an attribute being set to None in the same way can be handy (for generated config files mainly), and I think it's reasonable for something called is_configured to return True there. But I don't think the same treatment should be extended to False or other False-ish thing -- someone had to explicitly set the attribute to this value, so it definitely /is/ configured.

For instances where this is needed (treating a False-ish value the same way as a non-existent value), I'd just spell this out in code:

if is_configured('llvm_use_sanitizer') and config.llvm_use_sanitizer:
  ...