Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue with hard coded temp directory path #15

Open
guiveg opened this issue Dec 29, 2019 · 8 comments
Open

Issue with hard coded temp directory path #15

guiveg opened this issue Dec 29, 2019 · 8 comments
Assignees
Labels
bug Something isn't working feedback wanted help wanted Extra attention is needed

Comments

@guiveg
Copy link

guiveg commented Dec 29, 2019

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
Copy link

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

That is the error.

@Dapbler
Copy link
Owner

Dapbler commented Feb 24, 2020

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 Dapbler self-assigned this Feb 24, 2020
@Dapbler Dapbler changed the title Issue with temp directory in Mac OS Issue with hard coded temp directory path Feb 24, 2020
@Dapbler
Copy link
Owner

Dapbler commented Feb 24, 2020

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 Dapbler added bug Something isn't working enhancement New feature or request help wanted Extra attention is needed labels Feb 24, 2020
@Dapbler
Copy link
Owner

Dapbler commented Feb 25, 2020

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

@Dapbler Dapbler removed the bug Something isn't working label Feb 25, 2020
@PiDockMedia
Copy link

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
Copy link
Owner

Dapbler commented Jul 19, 2021

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
Copy link
Owner

Dapbler commented Jul 19, 2021

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 Dapbler added seeking confirmation bug Something isn't working feedback wanted and removed enhancement New feature or request seeking confirmation labels Jul 19, 2021
@Dapbler
Copy link
Owner

Dapbler commented Aug 11, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feedback wanted help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants