This is an archive of the discontinued LLVM Phabricator instance.

[lit] Add a shared llvm configuration library, and use it in llvm
ClosedPublic

Authored by zturner on Sep 12 2017, 4:09 PM.

Details

Summary

Building off of D37756, the next step is to take all the common, reusable stuff that is copied across the various LLVM projects, and centralize it.

I've identified two key areas that I'm tackling with this patch:

  1. Stuff that can be done automatically for every single LLVM project. For example, almost every test starts with 6-7 lines of copied code to add GnuWin32 to the path on Windows. Those that don't, probably should. Similarly, most of the projects add some common "features" that can be checked by tests. All of these features now get added automatically.

Caveat: In some cases this is introducing *new* features to projects that didn't currently have these features before. This is a change, but I think it's worth doing, as I only chose the most common, generic not-project-specific ones for this purpose.

  1. Adding some convenience functions that reduce the amount of code required to do various common things. For example, a lot of projects need to run llvm-config in various ways in order to determine if certain features are enabled. This involves duplicating the same 10 lines of code multiple times. Now, by using this helper function, it's 1 line.

At a high level, the way this works is to add a new llvm-specific submodule to lit. Then, you have your site config put the magic configure-like replacement and configure automatically imports and initializes this module for you. Then, in your main config, an object magically appears that you can just use.

With this patch I'm only introducing this into LLVM's lit.cfg. The idea is to get feedback on the high level ideas. I can go through and cleanup the remainder of the projects later.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.Sep 12 2017, 4:09 PM
dlj edited edge metadata.Sep 12 2017, 5:58 PM

Mostly just nit-picky Python comments... the helper class seems to reduce the overhead quite a bit.

llvm/utils/lit/lit/llvm/config.py
6 ↗(On Diff #114933)

It might make sense to move this into the LLVMConfig class, so you don't have to keep passing lit_config.

It also looks like this is always called with attributes on config, so you could roll in the getattr()s here as well.

21 ↗(On Diff #114933)

Nit: this should probably inherit from object for the benefit of py2.

class LLVMConfig(object):

39 ↗(On Diff #114933)

(Minor suggestion) It might make sense to decide the platform default earlier, then pass it as the default to get(). That should let you collapse both of the next blocks:

shell_default = ('0' if sys.platform not in ['win32'] else '')
if os.environ.get('LIT_USE_INTERNAL_SHELL', shell_default) == '0':
  self.use_lit_shell = False
else:
  self.use_lit_shell = True
  config.available_features.add('shell')

(Not sure if that's much clearer or not, especially with the comments.)

59 ↗(On Diff #114933)

If this value is set, will it be in CMake's internal semicolon-separated format? It might be better to split the value into a list if you can, instead of relying on substring searching.

60 ↗(On Diff #114933)

If you pass an iterable default to getattr() instead of None, you should be able to just omit this check.

61 ↗(On Diff #114933)

You could cut down on the duplication here with a lambda:

_check = lambda name, feature: ('' if name in use_sanitizer else 'not_') + feature
config.available_features.add(_check('Address', 'asan'))

75 ↗(On Diff #114933)

It looks like this should be have_zlib... but in any case, you could reduce duplication (and the potential for typops) by building the string directly with a ternary:

config.available_features.add(('' if getattr(config, 'have_zlib', None) else 'no') + 'zlib')

91 ↗(On Diff #114933)

(Side note: if you move _is_true_like into the class, you can probably just move the logic of this function into the if statement.)

109 ↗(On Diff #114933)

It would probably make more sense for this to be a default-valued argument:

def with_environment(..., append_path=False)

I don't see any reason that you would want to reprocess or pass along other kwargs, so the default valued option is probably clearer.

114 ↗(On Diff #114933)

Minor nit: maybe call the variable "prepend_path"?

118 ↗(On Diff #114933)

Are you sure it's safe normalize the existing values?

It seems like it should be OK, since you won't be re-normalizing all the time. Doing normalized comparisons would be a bit more complex, too.

118 ↗(On Diff #114933)

I think you want this to be:

os.path.pathsep.split(current_value)

119 ↗(On Diff #114933)

You can narrow the scope here:

try:
  items.remove(value)
except ValueError:
  pass
items = [value] + items
128 ↗(On Diff #114933)

Minor nit: 'vars' is a Python builtin name, maybe call it 'variables' instead?

129 ↗(On Diff #114933)

I think you want:

if isinstance(vars, str):
144 ↗(On Diff #114933)

Should this use self.config.fatal?

146 ↗(On Diff #114933)

I think you might want to use this pattern instead:

output, _ = llvm_config_cmd.communicate()
if re.search('ON', output.decode('ascii')):
  # ...

That will handle the blocking semantics correctly on the subprocess's file handles. It also eliminates the need for the wait() below.

zturner added inline comments.Sep 12 2017, 7:35 PM
llvm/utils/lit/lit/llvm/config.py
6 ↗(On Diff #114933)

Hmm, even better might be perhaps to move this into lit.util. There's nothing llvm specific about this

118 ↗(On Diff #114933)

Yea, the problem is that I really need to compare whether or not the paths are "equivalent". This means handling case-sensitivity and normalizing slashes, otherwise the string compare won't work. If someone puts D:/src/llvm into a variable, and then I add D:\src\llvm, they should match. Luckily this code isn't really in a hot path, so performance doesn't matter, and I can't think of a reason why it would be unsafe. On non-Windows platforms, norm is a no-op

I think you want this to be: os.path.pathsep.split(current_value)

Is there a difference?

146 ↗(On Diff #114933)

Good suggestion. It's almost as if centralizing all this copied code into a single place has tangible value :D

zturner updated this revision to Diff 114965.Sep 12 2017, 9:31 PM

Updated from suggestions

zturner updated this revision to Diff 114966.Sep 12 2017, 9:34 PM

A duplicated copy of the new files got added somehow in the last patch. Removed them.

dlj accepted this revision.Sep 14 2017, 5:34 PM
This revision is now accepted and ready to land.Sep 14 2017, 5:34 PM
This revision was automatically updated to reflect the committed changes.