Page MenuHomePhabricator

lit.py: Allow configs and local configs to have a setup_script entry
AbandonedPublic

Authored by jordan_rose on Sep 18 2017, 5:01 PM.

Details

Summary

If set, the contents of this field are run as a Python script before any tests within that directory are executed. This happens once per execution directory, so tests in subdirectories should take care. The execution of the setup script gets a global environment like lit.local.cfg, but with shared_output as an additional global. This is a path that can be shared by all files within a particular directory, located alongside the %t paths within the Output/ directories. It's also available in tests as %shared_output.

This is useful for families of tests that have some expensive setup, which would be bad to duplicate across tests.

Either this or D35396 will need to be rebased on top of the other.

Diff Detail

Repository
rL LLVM

Event Timeline

jordan_rose created this revision.Sep 18 2017, 5:01 PM

Why not just have lit.setup-folder.cfg similar to how we have lit.local.cfg?

Also, what about folder-level teardown? Would it instead be better to have the setup script provide an init and teardown function that get run at the beginning an end?

utils/lit/tests/Inputs/setup-script/subdir-custom/lit.local.cfg
2

Can you make this not be a shell script? This is essentially saying "This test doesn't work on Windows" when there's no particular reason why it shouldn't

zturner added inline comments.Sep 18 2017, 8:29 PM
utils/lit/tests/Inputs/setup-script/subdir-custom/lit.local.cfg
2

Actually I take that back. This can't be a shell script, because it's saying "this feature doesn't work on Windows". So I think this has to be a Python script, and you just execfile it. If you want to run a shell script, you can still do that by having a Python script which runs a shell script. But the lit infrastructure should only deal with Python scripts.

Actually, I guess I'm a bit confused. What is this solving that you can't already do in a local config? Couldn't you just embed the stuff you want to execute in the local config?

Actually, I guess I'm a bit confused. What is this solving that you can't already do in a local config? Couldn't you just embed the stuff you want to execute in the local config?

I felt uncomfortable making the local config files actually have side effects, but really I want to follow up with this by moving the setup tasks into the parallel processing as actual dependencies. This is just the first iteration of the feature. If you think that's not a good direction, though, we could go with one of the other options.

(This also allows for the inheritance of setup logic into subdirectories, but that's not a critical feature for my use case, and indeed it's likely unexpected in many cases.)

utils/lit/tests/Inputs/setup-script/subdir-custom/lit.local.cfg
2

I'm happy to make the tests use Python—good point—but I don't see why the feature wouldn't work on Windows. Windows can already run Python scripts from the command line, right?

zturner added inline comments.Sep 19 2017, 11:42 AM
utils/lit/tests/Inputs/setup-script/subdir-custom/lit.local.cfg
2

No, you need to specify the path to the python executable. You can't just run foo.py, you need to run C:\Python27\python.exe foo.py. People can make this work by modifying some environment variables or doing some other tweaking, but that won't work here because it's a feature of the shell, not the OS. And it could end up pointing to a different python executable than what they ran the test suite is. If everything is Python you can address this by using sys.executable.

That aside though, given that lit is intended to be cross-platform, it seems like it should actively encourage people to write only cross-platform tests. If you make it possible to execute shell scripts, people are going to make use of that functionality and then if they ever need to add portability down the line, it's going to be much harder.

jordan_rose added inline comments.Sep 19 2017, 11:48 AM
utils/lit/tests/Inputs/setup-script/subdir-custom/lit.local.cfg
2

Hm, I was treating the docs at https://docs.python.org/2/using/windows.html?highlight=windows#executing-scripts as saying that wasn't necessary, but you'd know better than me. The point about picking a different Python is taken, though.

That aside though, given that lit is intended to be cross-platform, it seems like it should actively encourage people to write only cross-platform tests.

This part seems silly to me. Plenty of test content isn't cross-platform, and the most common kind of lit test are RUN-based shell-like tests that can invoke anything in PATH. I don't think it's a goal of lit to keep people from writing non-portable tests.

That said, I'm fine with making this Python-only.

jordan_rose edited the summary of this revision. (Show Details)

Make setup_script refer to a Python script rather than an arbitrary executable, as suggested by @zturner.

Actually, I guess I'm a bit confused. What is this solving that you can't already do in a local config? Couldn't you just embed the stuff you want to execute in the local config?

I felt uncomfortable making the local config files actually have side effects, but really I want to follow up with this by moving the setup tasks into the parallel processing as actual dependencies. This is just the first iteration of the feature. If you think that's not a good direction, though, we could go with one of the other options.

(This also allows for the inheritance of setup logic into subdirectories, but that's not a critical feature for my use case, and indeed it's likely unexpected in many cases.)

This version is better, thanks for making it work with Python scripts.

I'm interested in getting Daniel's input, but I still find it a little strange that we have 2 features that essentially do the same thing. For example, let's imagine this setup script file contains a block of code X. Couldn't we just paste X into the lit.local.cfg and have it be functionally equivalent but with one less file for the user to deal with?

You mentioned you want to make this work with the multiprocessing pool. That worries me a little bit because different processes are going to be using the same shared output dir (which isn't created up front by the infrastructure), if I understand correctly. Like if you have A and A/B and A/C, then we will run the setup script 3 times in parallel all using the same shared output folder. Is this going to be a source of flakiness?

Still though, let's say we decide that's ok. In that case, I'm still curious how this would be functionally different from just pasting all the code into a lit.local.cfg. Does that not get run for each subdirectory?

You mentioned you want to make this work with the multiprocessing pool. That worries me a little bit because different processes are going to be using the same shared output dir (which isn't created up front by the infrastructure), if I understand correctly. Like if you have A and A/B and A/C, then we will run the setup script 3 times in parallel all using the same shared output folder. Is this going to be a source of flakiness?

Ah, that's not what happens. In the scenario you described, the script gets run for A/Output/Shared, A/B/Output/Shared, and A/C/Output/Shared. There's no feature (currently) for having a common %shared_output across multiple test directories because I didn't want to deal with synchronization. (Something like that would mean bringing back %T, though perhaps under a different name.)

You mentioned you want to make this work with the multiprocessing pool. That worries me a little bit because different processes are going to be using the same shared output dir (which isn't created up front by the infrastructure), if I understand correctly. Like if you have A and A/B and A/C, then we will run the setup script 3 times in parallel all using the same shared output folder. Is this going to be a source of flakiness?

Ah, that's not what happens. In the scenario you described, the script gets run for A/Output/Shared, A/B/Output/Shared, and A/C/Output/Shared. There's no feature (currently) for having a common %shared_output across multiple test directories because I didn't want to deal with synchronization. (Something like that would mean bringing back %T, though perhaps under a different name.)

I'm just brainstorming here, but what if a local config could say something like:

# lit.local.cfg
config.use_shared_output_folder('foo')

The implementation of this function would just find the output directory using the site config (which should already be created), and create this folder under there.

We only ever run the local config once, although we pass the resulting TestingConfig object to each subtest. So there would be no need to worry about parallelization.

This really only supports the single use case of a shared output directory though, and not running arbitrarily complex scripts. But if all you need is a shared output folder, maybe this is sufficient?

Oh, no, I definitely need an arbitrarily complex script. Swift wants to prebuild mock libraries shared by several test cases. Today we're either doing it once per test case or using a terrible hack mode that can parse source on the fly.

Alright then, if you need it you need it :)

Let's see if Daniel has any thoughts. I mildly prefer finding a way to keep everything in the lit.local.cfg, but if someone else feels strongly that it's the way to go, I don't think it's the end of the world.

One other idea would be something like

def setup_subdir():
  pass

config.setup_func = setup_subdir
zturner added a reviewer: rnk.Sep 19 2017, 4:39 PM

Yep, that's not bad either. I'm not super happy with the idea of lit.local.cfg parsing relying on running in the same process as lit itself, but maybe that ship has sailed.

Move setup code from execute_tests_in_pool to execute_tests.

This is always what I meant to do; I messed up when rebasing at one point. As mentioned in the discussion, I'm hoping to move these into being actual multiprocessing tasks in a follow-up commit.

zturner added inline comments.Sep 19 2017, 5:42 PM
utils/lit/lit/run.py
49

Maybe move this into a function such as

lit.util.run_inline_script(config, litConfig, setup_script, 
                           {'shared_output' : shared_output})

and update TestingConfig.load_from_path to call this same function?

jordan_rose abandoned this revision.Sep 22 2017, 4:25 PM

@ddunbar convinced me in person that this is either overkill or doesn't go far enough: as long as there's a shared directory, either there can just be a common setup task at the start of each test that does its own ad hoc filesystem-based synchronization, or we might as well go all the way to full-on inter-test dependencies. I'm going to split out the shared directory stuff into a separate patch and go play with the idea of dependencies for a bit.