LGTM modulo requested comments.
================
Comment at: test/lit.cfg:204
@@ -184,3 +203,3 @@
try:
- os.remove(exec_path)
+ self._clean(exec_path)
except:
----------------
danalbert wrote:
> 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.
I'll buy that. Can you add a comment so that is clear?
================
Comment at: test/lit.cfg:529
@@ -508,1 +528,3 @@
+ print 'Using configuration variant: %s' % cfg_variant
+configuration = globals()['%sConfiguration' % cfg_variant](lit_config, config)
configuration.configure()
----------------
danalbert wrote:
> EricWF wrote:
> > Is there a prettier way to right this line? Just so I understand
> > `globals()['%sConfiguration' % cfg_variant]` looks up and evaluates to a
> > type with the name `%sConfiguration`?
> I believe `getattr(__module__, '%sConfiguration' % cfg_variant)(lit_config,
> config)` would work too, but that isn't all that much better imo. At some
> point I'l like to teach LIT to add the directory of lit.cfg to `PYTHONPATH`
> so we can split some things out into different modules. Then this could be
> done in lit.site.cfg as `from android import AndroidConfiguration as
> Configuration` and lit.cfg would become
>
> try:
> configuration = Configuration(lit_config, config)
> except NameError:
> configuration = DefaultConfiguration(lit_config, config)
Please add a comment explaining this line.
http://reviews.llvm.org/D6373
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
