rec/safer

`safer` does not work on Windows

EdLipson5117 opened this issue · 17 comments

EdLipson5117 commented

The context manager I'm using for write:
with sopen(self.fitbitjsonfn, mode='wt', delete_failures=True) as jfile:
jdump(self.fitbitjson, jfile)

Error:
FileExistsError: [WinError 183] Cannot create a file when that file already exists: 'C:\Users\edlip\Documents\code\vsc_ws\fitbit_charge\src\main\resources\base\json\tmpcwbtnzjz' -> 'C:\Users\edlip\Documents\code\vsc_ws\fitbit_charge\src\main\resources\base\json\fitbit.json'

rec commented

Thanks for the clear report! I'll get on this right now.

You are as far as I know the first Windows user, so if it's a Windows thing, I might need to experiment a little with you.

Developing!

rec commented

Oh, if you had the whole stack trace, that would be great too.

rec commented

So it does seem to be a Windows thing - I just pushed a test for rewriting files and it seems to work. :-/

b58ed8f

The whole stack trace will be helpful, or I might push out a test version for you.

Also: delete_failures=True is the default, you don't need to set it.

with sopen(self.fitbitjsonfn, mode='wt', delete_failures=True) as jfile:

can just be

with sopen(self.fitbitjsonfn, 'wt') as jfile:
rec commented

Sorry for all the traffic, I hate bugs. :-D

I'm setting up with Appveyor which allows me Windows builds for CI. Really good to do anyway.

I only have a fairly limited slot to work on this today. I might get it done, or otherwise, tomorrow.

EdLipson5117 commented

Stack trace

Traceback (most recent call last):
File "C:\Users\edlip\AppData\Local\Programs\Python\py8Test1\lib\site-packages\safer.py", line 188, in safer_close
os.rename(temp_file, name)
FileExistsError: [WinError 183] Cannot create a file when that file already exists: 'C:\Users\edlip\Documents\code\vsc_ws\fitbit_charge\src\main\resources\base\json\tmp19i4hg6w' -> 'C:\Users\edlip\Documents\code\vsc_ws\fitbit_charge\src\main\resources\base\json\fitbit.json'

Python 3.8.2, venv list:
Package Version

-fxparse 0.20
altgraph 0.17
astroid 2.3.3
atomicwrites 1.3.0
attrs 19.3.0
autopep8 1.5
Babel 2.8.0
base36 0.1.1
beautifulsoup4 4.8.2
cffi 1.14.0
colorama 0.4.3
cryptography 2.8
et-xmlfile 1.0.1
fbs 0.8.4
future 0.18.2
fuzzywuzzy 0.18.0
isort 4.3.21
jdcal 1.4.1
lazy-object-proxy 1.4.3
lxml 4.5.0
mccabe 0.6.1
money 1.3.0
more-itertools 8.2.0
numpy 1.18.3
ofxtools 0.8.20
openpyxl 3.0.3
orderedset 2.0.3
packaging 20.1
pefile 2019.4.18
Pillow 7.1.1
pip 20.1
pluggy 0.13.1
py 1.8.1
py7zr 0.6
pyAesCrypt 0.4.3
pycodestyle 2.5.0
pycparser 2.20
pycryptodome 3.9.7
PyInstaller 3.6
pylint 2.4.4
pyparsing 2.0.2
pypiwin32 223
PyQt5Designer 5.14.1
PySide2 5.14.2.1
pytest 5.3.5
python-dateutil 2.2
python-Levenshtein 0.12.0
pytz 2019.3
pywin32 227
pywin32-ctypes 0.2.0
rope 0.16.0
safer 3.0.0
setuptools 41.2.0
shiboken2 5.14.2.1
six 1.14.0
soupsieve 2.0
sqlite3-backup 0.3.0
sqlparse 0.3.1
texttable 1.6.2
torch 1.5.0+cu92
torchvision 0.6.0+cu92
wcwidth 0.1.8
wrapt 1.11.2
wsgi-intercept 0.6.5
xmltodict 0.12.0

rec commented

Oh, don't close this! :-o

EdLipson5117 commented

No rush in fixing it, it is just for me and switching back to plain open is simple

EdLipson5117 commented

Hit the wrong button

  • 😄1
rec commented

Well, you won't be the last I hope!

Unfortunately, reading Appveyor's logs are daunting - because there are multiple issues.

https://ci.appveyor.com/project/rec/safer/builds/32685082/job/02opb49eud7cdt0k

  1. Cannot create a file when that file already exists: this is to do with os.rename() but what?

  2. UnicodeDecodeError: 'charmap' codec can't decode byte 0x8f in position 2: character maps to - probably due to reading UTF-8 from that file, but that test doesn't even need to be enabled for windows.

  3. But then tons of things like this: AssertionError: 'hello' != 'OK!'

Which means that is just "doesn't work" - it gives the wrong answers.

Clearly there are a lot of ideas to do with Windows files I just don't understand.

I'm going to do some research; it might be some time though.

EdLipson5117 commented

Not sure if it applies but while researching this before opening the issue I spotted this in the built-in open
Changed in version 3.3:
The opener parameter was added.

The 'x' mode was added.

IOError used to be raised, it is now an alias of OSError.

FileExistsError is now raised if the file opened in exclusive creation mode ('x') already exists.

rec commented

Sorry I didn't respond, Ed - but I still don't have any good solution for Windows development here.

Until I either get a Windows machine or a Windows collaborator, I'm probably not going to be able to fix this. :-/

I released version 4 with a lot of new things in it and by popular demand, by default everything is now memory-cached, not file cached, so that might work better for you.

FMeinicke commented

os.replace instead of os.rename seems to be the solution for Windows. It's supposed to work the same as os.rename on POSIX. According to the stdlib documentation of os.rename:

If you want cross-platform overwriting of the destination, use replace().

rec commented

I'll bet that would work, but it'd be great to actually test it on Windows somehow!

I'll make that change anyway, it can hardly hurt.

FMeinicke commented

Thanks for the quick change. I can confirm that this works like a charm on both Windows and Linux (at least for my use case: with safer.open(file_path, "w", delete_failures=False) as file:).

Would you mind creating a release for this change so I can install safer from pypi?

rec commented
  • 🚀1