I noticed that things in livecd/rm like "/boot/System*" are not working. It seems that os.path.exists in python doesn't expand globs/wildcards. 13 Jul 2018 09:56:55 CDT: NOTICE : log.py:notice: --- Running action sequence: remove 13 Jul 2018 09:56:55 CDT: NOTICE : log.py:notice: livecd: removing /boot/System* 13 Jul 2018 09:56:55 CDT: DEBUG : fileops.py:clear_dir: start: /mnt/btr/catalyst/tmp/default/livecd-stage2-amd64-latest/boot/System* 13 Jul 2018 09:56:55 CDT: DEBUG : fileops.py:clear_dir: Condidtions not met to clear: /mnt/btr/catalyst/tmp/default/livecd-stage2-amd64-latest/boot/System* 13 Jul 2018 09:56:55 CDT: DEBUG : fileops.py:clear_dir: isdir: False 13 Jul 2018 09:56:55 CDT: DEBUG : fileops.py:clear_dir: islink: False 13 Jul 2018 09:56:55 CDT: DEBUG : fileops.py:clear_dir: exists: False 13 Jul 2018 09:56:55 CDT: DEBUG : fileops.py:clear_dir: DONE, returning True # ls /mnt/btr/catalyst/tmp/default/livecd-stage2-amd64-latest/boot/System* /mnt/btr/catalyst/tmp/default/livecd-stage2-amd64-latest/boot/System.map-genkernel-x86_64-4.14.52-gentoo #
I believe this extends to any file - the handling in fileops.py:clear_dir doesn't appear to take account of anything besides directories (shutil.rmtree) and links (os.unlink). I propose the creation of an auxiliary function 'clear_files' which is called from a separate test within the if 'tree' https://gitweb.gentoo.org/proj/catalyst.git/tree/catalyst/fileops.py#n72 before the else at https://gitweb.gentoo.org/proj/catalyst.git/tree/catalyst/fileops.py#n94 with a glob.glob call to see if any files are returned. These can then be removed with the customary os.remove function. I don't see any need to retain the functionality of changing permission/ownership of individual files (although I will be advised on this) in this new function, but it depends whether there are any other calls to 'clear_dir'a side from the 'rm' process. First-cut patch will be uploaded shortly.
Created attachment 539448 [details, diff] proposed clear_files function *BEWARE* this is totally UNTESTED - just posting to give a flavour of my thinking at present.
+ try: + log.debug("os.remove()") + os.remove(myfile) + except Exception + log.error('clear_files failed', exc_info=True) + return False Some initial review... 1) don't put the log.debug in the try: except: 2) include the file it is trying to remove 3) don't do all encompassing Exception, in this case OSError would be appropriate. 4) Don't return from the exception, that breaks out of the loop at the first sign of trouble. It would be better to set a succees=True beofre the loop, set a success=False in the exception. Then return success. That way it will remove everything it can and return True if all was well, False if there was any failure.
Many thanks for the review, Brian. I'll do some more work on it over the next couple of days, and then submit to the mailing-list for final tweaks and approval.
Created attachment 539960 [details, diff] updated patch This is the updated patch that veremitz emailed for review, attaching it here for more eyeballs
Also FYI I have been testing builds w/ this patch all week with good results (though I cannot comment on correctness of the python code itself)
code looks perfect. I didn'tget the email...
(In reply to Brian Dolbec from comment #7) > code looks perfect. > > I didn'tget the email... sent to wrong list, that's why! :)
Fixed in 3.0.2
