This is an archive of the discontinued LLVM Phabricator instance.

[lit] Allow lit config files to have a .py extension
ClosedPublic

Authored by zturner on Sep 13 2017, 5:22 PM.

Details

Summary

Not having a .py extension breaks many python related tools, and is generally kind of obnoxious.

This is a minor quality of life improvement to allow lit config files to have .py extensions. The original filenames are still supported, now you just have the *option* of having a lit config file with a .py extension. I didn't want to get too heavy handed, so I only changed the configs in llvm/test and llvm/test/Unit

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.Sep 13 2017, 5:22 PM
rnk edited edge metadata.Sep 13 2017, 5:32 PM

lit.cfg.py still isn't a valid name for a python module. I had this idea that in the future we'd import the config module directly to simplify custom test formats defined in lit configuration files, which interact badly with multiprocessing.Pool. I guess that won't work well anyway since all the config files will have the same name, so we still have to move custom test formats out into well-named python modules.

In D37838#870368, @rnk wrote:

lit.cfg.py still isn't a valid name for a python module. I had this idea that in the future we'd import the config module directly to simplify custom test formats defined in lit configuration files, which interact badly with multiprocessing.Pool. I guess that won't work well anyway since all the config files will have the same name, so we still have to move custom test formats out into well-named python modules.

Could we work around this by using __import__ or something or searching for a module loader with a given path, and just assigning a dynamically generated name to the module? Not trying to do anything like that with this patch by the way.

If it makes things simpler, I could call it lit_cfg.py?

rnk added a comment.Sep 13 2017, 6:14 PM
In D37838#870368, @rnk wrote:

lit.cfg.py still isn't a valid name for a python module. I had this idea that in the future we'd import the config module directly to simplify custom test formats defined in lit configuration files, which interact badly with multiprocessing.Pool. I guess that won't work well anyway since all the config files will have the same name, so we still have to move custom test formats out into well-named python modules.

Could we work around this by using __import__ or something or searching for a module loader with a given path, and just assigning a dynamically generated name to the module? Not trying to do anything like that with this patch by the way.

I think we could do something crazy like that.

If it makes things simpler, I could call it lit_cfg.py?

It probably won't make the import stuff simpler, so leaving it as lit.cfg.py seems more consistent with our current lit.site.cfg naming.

dlj accepted this revision.Sep 14 2017, 2:46 PM
In D37838#870412, @rnk wrote:
In D37838#870368, @rnk wrote:

lit.cfg.py still isn't a valid name for a python module. I had this idea that in the future we'd import the config module directly to simplify custom test formats defined in lit configuration files, which interact badly with multiprocessing.Pool. I guess that won't work well anyway since all the config files will have the same name, so we still have to move custom test formats out into well-named python modules.

Could we work around this by using __import__ or something or searching for a module loader with a given path, and just assigning a dynamically generated name to the module? Not trying to do anything like that with this patch by the way.

I think we could do something crazy like that.

If it makes things simpler, I could call it lit_cfg.py?

It probably won't make the import stuff simpler, so leaving it as lit.cfg.py seems more consistent with our current lit.site.cfg naming.

Just to throw my hat into the ring here...

I don't think the structure of the lit.cfg, et. al. files are terribly amenable to being imported in Python. The lit configs are written as fallthrough scripts, and probably have too many assumptions about global state and global variables to make sense to import them using Python's import mechanics. They need to work more like a pasted textual inclusion, so they can run after the basic environment is set up, but within the pre-populated namespace. They don't really work as a standalone namespaced module, and the assumed presence of globals are (I think) the main showstopper.

From that perspective, the illegal naming is almost more of a feature than a drawback. (I do think, however, that adding the extension for purposes of filename associations and editor support is reasonable.)

This revision is now accepted and ready to land.Sep 14 2017, 2:46 PM
This revision was automatically updated to reflect the committed changes.
llvm/trunk/test/lit.cfg.py