[PATCH] [libcxx] Add better support for custom test runners.

Dan Albert danalbert at google.com
Mon Nov 24 13:49:05 PST 2014


================
Comment at: test/lit.cfg:204
@@ -184,3 +203,3 @@
                 try:
-                    os.remove(exec_path)
+                    self._clean(exec_path)
                 except:
----------------
EricWF wrote:
> danalbert wrote:
> > EricWF wrote:
> > > danalbert wrote:
> > > > EricWF wrote:
> > > > > `exec_path` is always created so it should always be removed.  If creating the file doesn't belong to a customization point then neither should cleaning it up. 
> > > > > 
> > > > > Small nit: Will there ever be a case we want `_clean(...)` to throw? Should exception handling be the responsibility of `_clean(...)`?
> > > > The build step is the one that creates the file, and that _is_ customizable. Remote test executors need to customize the cleanup step because they need to remove the output from both the compilation location and the run location.
> > > > 
> > > > Yeah, I'm thinking `_clean()` worries about the exceptions, since the action taken might vary based on the internals (adb fails often, so probably just retry, whereas `os.remove()` will probably only fail for bad permissions, in which case just ignore).
> > > Isn't the file created by `exec_file = tempfile.NamedTemporaryFile(suffix="exe", delete=False)` on line 170? The `_build` step just overwrites it.
> > Still potentially have to deal with cleanup up remote paths. Also, in Android we generate a .o before linking (this is what our build system does, and I want to test our builds exactly).
> Sure, I'm not saying scrap `_clean` as a customization point. I'm just wondering if we should always try and clean up `exec_path` since we always create it.
Eh. If you're overriding `_clean()`, it's your responsibility to call `super` if you want it to do the same things.

http://reviews.llvm.org/D6373






More information about the cfe-commits mailing list