Dapbler / cbr2cbz

Python script that converts compressed CBR and CBZ comic archives to stored CBZ. Many options.
GNU General Public License v3.0
26 stars 5 forks source link

Issue with hard coded temp directory path #15

Open guiveg opened 4 years ago

guiveg commented 4 years ago

I am testing cbr2cbz with a Mac OS. When I tried to run it with a sample file, I get the following error:

Creating /tmp/cbr2cbztemp-u501/p1292 Could not change to temp directory /tmp/cbr2cbztemp-u501/p1292

The directory is nevertheless created

PiDockMedia commented 4 years ago

Creating /tmp/cbr2cbztemp-u501/p33649 Could not change to temp directory /tmp/cbr2cbztemp-u501/p33649

That is the error.

Dapbler commented 4 years ago

I'll add in the suggestion from issue #16 - an option to set the temporary folder.

Looks like my working tree of the script (at home, I don't think it went up to Github) has different imagemagick commands (updated packages in Ubuntu Linux). I may need to add in an option for different command styles and try to work out why they changed.

If there's a lot of variation between OSes there may be a need for short term aliasing of commands, and increased priority of redoing graphic conversion (primarily shrinking) with python library.

Once the temp folder flag is in place I will probably need some feedback as to if the imagemagick is available/working on MacOS

Dapbler commented 4 years ago

The hard coded temp directory path is an issue on more than MacOS, so I've renamed the issue and will close #16 as a planned workaround. On my end /tmp was originally using tmpfs (a RAM hosted filesystem), but somewhere since I've moved to Ubuntu 19.10 where it expects you to use /run/user/userid/ for tmpfs - so having it hard coded to /tmp isn't good either.

I'll have to look into how to ask python what OS is being used and try to work out sensible defaults. Hopefully someone's already has good advice elsewhere.

Dapbler commented 4 years ago

I've added a --tempdir option to the Test branch. Try it out with somewhere you know you can write to. If that doesn't work there must be something incompatible with MacOS with how I'm checking directory changing/existing that'll need more investigation.

Note with --shrink I've reverted to ImageMagick6 command format by default (current on my Ubuntu 19.10). If using ImageMagick 7 use --imversion 7

PiDockMedia commented 4 years ago

The new tempdir option seems to work well. Interesting artifact, if I try and run it on the root of my ramdisk (or any disk) I get the errors below. If I declare a folder such as --tempdir /Volumes/ramdisk/tmp-$$ it creates it and removes it at the end. Maybe even with the --tempdir option it should assume it needs to create and delete a subdirectory to work in?

$ cbr2cbz -c --tempdir /Volumes/ramdisk ./_testcbr2cbz/ ./_testcbr2cbzdest/
Traceback (most recent call last):
  File "/scripts/cbr2cbz", line 889, in <module>
    main()
  File "/cbr2cbz", line 879, in main
    cbr2cbzclean(create=False,delete=True)
  File "/scripts/cbr2cbz", line 43, in cbr2cbzclean
    shutil.rmtree(cbr2cbztemp)
  File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.7/lib/python3.7/shutil.py", line 495, in rmtree
    onerror(os.rmdir, path, sys.exc_info())
  File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.7/lib/python3.7/shutil.py", line 493, in rmtree
    os.rmdir(path)
OSError: [Errno 16] Resource busy: '/Volumes/ramdisk'
Dapbler commented 3 years ago

Well, I sort of forgot Github existed for a while. Having a look at the code it expects that tempdir is a dedicated folder, the first thing it tries to do is clear out any contents. When it's pointed to /Volumes/ramdisk/ it tries to delete everything in there first (which it probably doesn't have permissions to do).

I think this needs an update to create a subfolder even when it's explicity given a folder to work with. If someone tells it the tempdir is ~/ it'll probably go and delete as much of the user's home directory as it can.

I'm thinking of having a list of default sensible temporary locations for as many OSes as people can suggest and have it check each in order until it finds one - then create sub directories in there.

EDIT: The tempdir option didn't make it into the main branch or releases with the old (dangerous) behaviour. After a bit of reworking it's in main now, with --tempdir being used to point to an existing directory in which a temporary one is created, used and removed.

Dapbler commented 3 years ago

New behaviour:

Looks for the following folders in order: 1) --tempdir option 2) /run/user/UID# 3) /tmp 4) ~

Whichever exists has a cbr2cbz.PID# subdirectory created, used as temp and then cleaned up.

Promoted to default branch after some modest testing.

EDIT: Also added the current directory to the error line that started this issue - if it's still an issue it should at least tell us what it's expecting and what the current directory is.

Dapbler commented 3 years ago

Update for Issue 15 adds UID to temp directory name to help with potential collisions in shared folders.