slave.py checks incorrect option when removing temp directory

[cwiddofer]

Starting at line 381 of slave.py:

if not options.work_dir:
        log.debug('Removing temporary directory %s' % slave.work_dir)
        shutil.rmtree(slave.work_dir)

This is for removing the files after the build, correct? So it should be checking whether options.keep_files is set rather than options.work_dir:

if not options.keep_files:
        log.debug('Removing temporary directory %s' % slave.work_dir)
        shutil.rmtree(slave.work_dir)

[osimons]

Confirmed. I'll fix.

[osimons]

Committed in [644]. Thanks!

[Olaf Meeuwissen <olaf@…>]

Sure you want to remove the work_dir and not merely the build_dir which gets created below the work_dir? This may be an issue when you specified the same --work-dir value for parallel bitten-slave processes.

[osimons]

Replying to Olaf Meeuwissen <olaf@…>:

Sure you want to remove the work_dir and not merely the build_dir which gets created below the work_dir?

Not as sure now as I was when I committed it... I suppose many use fixed work_dir to store things like pre-build checkouts and other dependencies - or even data. Deleting that is not so good.

I suppose the order of checking and action really should be:

if not options.keep_files:
        log.debug('Removing build directory %s' % slave.build_dir)
        shutil.rmtree(slave.build_dir)
if not (options.work_dir or options.keep_files):
        log.debug('Removing working directory %s' % slave.work_dir)
        shutil.rmtree(slave.work_dir)

Does that seem more suitable?

[osimons]

No, that is not quite right either. The build files are already ckecked against keep_files elsewhere in slave.py, so I would propose just make the lines in question look like:

if not (options.work_dir or options.keep_files):
        log.debug('Removing working directory %s' % slave.work_dir)
        _rmtree(slave.work_dir)

That should account for keeping files either if a custom working directory is used, or if specifically requested to keep files.

(Also noticed that this code uses shutil.rmtree() and not the Bitten (custom _rmtree()` - should use the same.)

[osimons]

Last attempt committed in [650]. Hope this works as expected for all now.

[Olaf Meeuwissen <olaf@…>]

Replying to osimons:

Last attempt committed in [650]. Hope this works as expected for all now.

Yup, that seems alright. At least now I won't wipe my build user's $HOME directory anymore when I forget to --keep-files ;-)