lilydjwg / nvchecker

New version checker for software releases
MIT License
428 stars 69 forks source link

Use `shutil.move()` instead of `os.rename()` #265

Closed guihkx closed 6 months ago

guihkx commented 6 months ago

Because os.rename() fails when renaming files across different filesystems.

This issue can be easily triggered if we mount the newver file as a volume in Docker, and then run nvchecker.

For example:

$ docker run -v ./source.toml:/source.toml \
             -v ./old.json:/old.json \
             -v ./new.json:/new.json \
             localhost/nvchecker -c /source.toml
[I 04-23 13:47:18.537 core:408] mysource: updated from 1.0.0 to 1.0.1 taskName=Task-2
Traceback (most recent call last):
  File "/usr/bin/nvchecker", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/lib/python3.12/site-packages/nvchecker/__main__.py", line 96, in main
    core.write_verfile(newverf, vers)
  File "/lib/python3.12/site-packages/nvchecker/core.py", line 167, in write_verfile
    safe_overwrite(file, data)
  File "/lib/python3.12/site-packages/nvchecker/core.py", line 128, in safe_overwrite
    os.rename(tmpname, resolved_path)
OSError: [Errno 16] Resource busy: '/new.json.tmp' -> '/new.json'
guihkx commented 6 months ago

Here's a Dockerfile, if you want to reproduce the issue for yourself:

FROM alpine:edge
WORKDIR /
ENV PIPX_BIN_DIR /usr/bin
RUN apk add --no-cache python3
RUN apk add --no-cache --virtual .build-dependencies build-base curl-dev git pipx python3-dev && \
    pipx install git+https://github.com/lilydjwg/nvchecker.git@f1ff604b4cb012ea85e6244134808d49c3797b16 && \
    apk del .build-dependencies
RUN nvchecker --version
ENTRYPOINT ["nvchecker"]

Build with:

docker build -t nvchecker -f Dockerfile .

And run like this (adjusting the paths of sources.toml, old.json and new.json, if necessary):

docker run -v ./sources.toml:/sources.toml \
    -v ./old.json:/old.json \
    -v ./new.json:/new.json \
    localhost/nvchecker -c sources.toml
lilydjwg commented 6 months ago

This is intentional to guarantee that either the old file or the new one (or both) is kept on disk in case of a crash (and whoever opens the file will see a fully-written version, although this is not useful for nvchecker). Bind-mounting data files is not supported. Please bind mount a directory instead.

guihkx commented 6 months ago

Oops, I honestly didn't even know it could work just with bind-mounted directories...

Thanks for that, and sorry for the hasty PR.