<html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div class="">Hi All,</div><div class=""><br class=""></div><div class="">After reading through this email thread and the discussion on the initial revision again, I have developed a bit of an opinion of my own.</div><div class=""><br class=""></div><div class="">TL;DR:</div><div class="">Reid already said:</div><div class="">> Fewer settings or modes of operation is better.</div><div class="">I agree with Reid; ideally this shouldn't have a configuration option.</div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">Long version:</div><div class="">We are now defining two different modes of execution, which I think could be a source of confusion.</div><div class=""><br class=""></div><div class="">If I read it right, then the main concerns w.r.t. this feature were:</div><div class="">1. Possibility of non-termination (currently mitigated via recursion limit)</div><div class="">2. Performance</div><div class="">3. Complicates debugging</div><div class=""><br class=""></div><div class="">I think all of these can be addressed/mitigated:</div><div class="">1. Non-termination can be solved in a more principled way (by sorting substitutions or another similar algorithm).</div><div class=""><br class=""></div><div class="">2. I don't think performance will be an issue, but let's measure to make sure. Louis already timed a complete run for libc++. In addition, we can measure `env LIT_OPTS=--no-execute ninja check-all`. `--no-execute` skips executing the actual tests, so we should get a good sense of lit overhead.</div><div class=""><br class=""></div><div class="">3. Debugging is a valid concern I think, especially because it doesn't have a purely technical solution. However, IMO it has been given too much weight. Let me try to explain my thoughts: the person doing the debugging is either aware of recursive expansion of substitutions or they aren't. If you don't know what to look for, i.e., you don't know that your are in the "recursive expansion" mode, there is not much we can help with. This can always happen and is even more insidious exactly because we have two different modes. For experts, we can provide an ergonomic debugging helper, e.g., `print(config.expand('%clang'))`.</div><div class=""><br class=""></div><div class="">On the other hand, the configuration option adds complexity:</div><div class="">We had a long discussion where it should reside (I do think TestingConfig is the right place) and the infrastructure for the mode is almost as much code as the feature itself. Not making this a configuration option sidesteps these issues.</div><div class="">Also, complexity tends to accumulate over time. A good "bad example" from the past: individual test timeouts, which sounds like a small, innocent feature. We now have `maxIndividualTestTimeIsSupported` which has a special implementation for AIX! (<a href="https://reviews.llvm.org/D64251" class="">https://reviews.llvm.org/D64251</a>)</div><div class=""><br class=""></div><div class="">Since (1, 2) have a purely technical solution, and (3) the "debuggability" argument is messy IMO (introducing separate modes has it's own downsides), I vote for not making this configurable at all.</div><div class=""><br class=""></div><div class="">I talked to Louis. He proposed the following plan and is willing to the work:</div><div class="">1. Implement longest-match matching of substitutions (this is a useful improvement in itself)</div><div class="">2. Address non-termination in a more principled way (by sorting substitutions or another similar algorithm)</div><div class="">3. Measure performance</div><div class="">4. Provide ergonomic helper to print expanded substitutions to aid debugging</div><div class="">5. Remove option in the config, turn on by default</div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">My opinion on related issues:</div><div class=""><br class=""></div><div class=""><i class="">Prefix matching</i></div><div class="">E.g, `%clang_asan` expanding for `%clang`, two solutions:</div><div class="">1) Use unique identifier, e.g., `%{clang}` or `%clang%`</div><div class="">2) Implement longest match</div><div class=""><br class=""></div><div class="">I prefer 2), because again it's a technical solution, while 2) is more convention-based (human) and would require a lot of changes to widely adopt.</div><div class=""><br class=""></div><div class=""><i class="">Ordering of substitutions</i></div><div class="">I think the order of substitutions should not matter. IMO, this is just another source of confusion.</div><div class="">Ideally substitutions would be a dictionary (unique keys) instead of a list of tuples.</div><div class=""><br class=""></div><div class="">Let me know what you think!</div><div class=""><br class=""></div><div><br class=""><blockquote type="cite" class=""><div class="">On Apr 1, 2020, at 7:01 AM, Louis Dionne <<a href="mailto:ldionne@apple.com" class="">ldionne@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br class="Apple-interchange-newline"><br class=""><blockquote type="cite" class=""><div class="">On Apr 1, 2020, at 01:35, Dan Liew <<a href="mailto:dliew@apple.com" class="">dliew@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;"><div class=""><blockquote type="cite" class=""><br class=""><div class="" style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;"><br class=""></div><div class="" style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;">Thanks for your reply. So your opinion is that my proposed (2) is the right way forward. I still (weakly) think that we made the right design choice because whether to recursively expand or not seems like a property or the test suite as a whole, and not really something that you'd want to customize on a per-directory basis. It's kind of like the timeout time on tests or whether to be verbose.</div><div class="" style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;"><br class=""></div></blockquote><div class=""><br class=""></div><div class="">But our closet proxy for "test suite as a whole" are the TestingConfig objects (they are per test suite directory). Putting the property in LitConfig was never the right design (again sorry I didn't spot this) because that is global to all test suites which is wrong. Given that a test suite can customise its substitutions on a per-directory basis it makes sense that it would be technically possible to configure the substitution recursive expansion on a per-directory basis too. I'm not saying doing that would be a good idea but allowing this is at least consistent with what we already allow users of llvm-lit to do.</div><div class=""><br class=""></div><div class="">It's not like the test timeout or the verbosity. Both of those are properties global to the lit invocation and thus global across all test suite. In both cases it's this way **because** these properties can be set by a command line flag that is global over the lit invocation. When I implemented per-test-timeout, the fact I added it to the LitConfig object rather than TestingConfig was a design mistake that I didn't realise I had made until we got into this discussion.</div><div class=""><br class=""></div><br class=""><blockquote type="cite" class=""><div class="" style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;">It seems to me like the root cause really is that we can modify the LitConfig globally from subdirectories. Consider for example the timeout time -- imagine what would happen if libc++ set the timeout time to e.g. 2 minutes because <reasons>. Then, imagine that the Clang config sets the timeout time to 10 seconds, because it doesn't make sense for Clang tests to take longer than that. In the current state of things, whichever lit.cfg file is processed last will win, which seems brittle. I believe we should either not share the global LitConfig instance (by copying it when we process subdirectories), or make it read-only after construction to make it clear that it shouldn't be modified. And if it was made read-only, then I agree it would make sense to move `recursiveExpansionLimit` to TestingConfig instead.</div><div class="" style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;"><br class=""></div><div class="" style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;">What do you think?</div></blockquote><br class=""></div><div class="">As I said above. When I implemented lit's per test timeout I made a design mistake putting it in the LitConfig object. It should be in TestingConfig. I think it would be extremely risky to move to a model where LitConfig objects get copied because there might be existing lit configs that rely on this behaviour.</div></div></div></blockquote><br class=""></div><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class="">Okay, I'll move it to the TestingConfig.</div><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br class=""></div><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class="">Louis</div></div></blockquote></div><br class=""></body></html>