Porting portalocker to use msvcrt instead of win32con

[techtonik]

If you compare http://code.activestate.com/recipes/65203-portalocker-cross-platform-posixnt-api-for-flock-s/ with file in this repo, you will notice a lot of meaningless changes. I'd revert the quotes back to original, because there are a lot of portalocker copies out there that needs to be synced.

[wolph]

If you have any changes you would like to get merged with the script please send your version (or preferably, the diff of your changes) and I will merge them. Or even better, create a pull request.

This project is as far as I know the only script like this which is actively maintained, fully pep8 and pyflakes compliant. The quotes are just a personal preference which (in case of merging) can be fixed by a simple search/replace so that shouldn't be too much of a problem :)

[techtonik]

They are already a problem. I can not compare your changes with mine and original script without replacing them first It is not a cool activity at all.

[techtonik]

I don't see a way to attach files here.

[techtonik]

So there are at least two project where this script is not only actively maintained, but also more convenient for maintainers:

• single file
• clear changes history from the original
• easy comparison

I think you can find more examples merged from AS.

[wolph]

Respectfully, if you are forking a version of the script which is over 3 years old you cannot expect to merge cleanly regardless. The last commit in which quoting was changed (midly I might add) is here: 602818f

The code refactoring done a few months ago have much greater impact on the ability to merge, but that's what refactoring does.

I will try to merge your changes but looking at the diffs there are a lot of changes but the quoting is not the problem.

[wolph]

Didn't mean to close this :)

[wolph]

Please inspect the diffs carefully, you will see that quotes are not the issue here. There has been only 1 minor quote changing diff since the initial release and those tiny changes are not what are causing the merge conflict here.

As for attaching files, the Gist system is quite handy for that :)
https://gist.github.com/

I'm going to try and merge it all now

[wolph]

I've looked through the diffs but I am not entirely sure what you would like to merged from it.

Since it also involves a few backwards incompatible changes like returning False instead of an exception when something goes wrong I cannot merge it in the current state.

Do you have a diff of what you have actually changed and what you would like to be added?

[techtonik]

No, sorry, and I might not have time for this soon. I just want to make sure that the feedback reaches the maintainer, so that future portalocker changes are easier to merge back into SCons and Django.

[techtonik]

If that's possible. That's it.

[techtonik]

I am not quite sure I like that portalocker is not self-contained script now. It is convenient if you can just fetch and commit it into project without all this dependency management. It is also would be nice to maintain a list of differences between AS forks (API and approaches) with strong and weak points taken, incompatibilities and reasoning. Diagrams? That could be a very interesting project. =) Not sure I will find the time then, but that would at least enable me to monitor the progress. =)

[wolph]

After a bit of Googling I found these changes: http://sourceforge.net/p/roundup/code/ci/5e24a2d7078f199b45380328da2bf498480e3c0e/log/?path=/roundup/backends/portalocker.py

So can I assume that you want these changes to be merged?
http://sourceforge.net/p/roundup/code/ci/4e740f02e16573891968ecde086e7c1a7063fea1/tree//roundup/backends/portalocker.py?diff=6e3e4f24c75376f61ae0bf0e9ee334567585c38e

In that case I have a question about the code. With the new version the constants are hardcoded locally instead of being imported, is this a documented system call somewhere so we can link to the documentation just in case it would ever change?

For example:

    import win32con
    import win32file
    import pywintypes
    LOCK_EX = win32con.LOCKFILE_EXCLUSIVE_LOCK
    LOCK_SH = 0 # the default
    LOCK_NB = win32con.LOCKFILE_FAIL_IMMEDIATELY
    # is there any reason not to reuse the following structure?
    __overlapped = pywintypes.OVERLAPPED()

Versus:

    import msvcrt
    from ctypes import *
    from ctypes.wintypes import BOOL, DWORD, HANDLE

    LOCK_SH = 0    # the default
    LOCK_NB = 0x1  # LOCKFILE_FAIL_IMMEDIATELY
    LOCK_EX = 0x2  # LOCKFILE_EXCLUSIVE_LOCK

[techtonik]

These constants are from C includes of Windows API.

[wolph]

Since you are already using msvcrt, Is there still any reason for manually locking?
The library has locking support built-in doesn't it? http://docs.python.org/2.6/library/msvcrt.html#msvcrt.locking

[techtonik]

Good catch. I think the reason is that I didn't notice what was written above when I was looking for ctypes replacement as http://docs.python.org/2.6/library/msvcrt.html#msvcrt.get_osfhandle

I might ask the same question - why people didn't use it before?

[wolph]

Although I cannot say for certain, I expect this to be attributable to the fact that this module was originally written for Python 1.5.2, back in those days there wasn't a msvcrt library I guess.

With the lack of a Windows machine I can't say for sure that I can write and test this module though. But I will see what I can do with Wine and/or Virtualbox.

[techtonik]

If your tests are portable and you can wrap them into "checkout & run" style I might be able to find a machine to run them. No time for the brain work though.

[wolph]

Not sure if it's a good move though, it appears that the web2py fork switched to msvcrt some time ago and switched back for some reason:
Switch towards msvcrt: https://code.google.com/p/web2py/source/diff?path=/gluon/portalocker.py&format=side&r=c26241f7e1afa4fd5ef3cffa2a206ecbeef9eb25&old_path=/gluon/portalocker.py&old=6e5c994eba5aed464bdc1ef55f5c8cb1dc1010f6
Switch back: https://code.google.com/p/web2py/source/diff?spec=svn2ebf6e18ab822a6dca69dc2508c963d8ab67c98b&r=2ebf6e18ab822a6dca69dc2508c963d8ab67c98b&format=side&path=/gluon/portalocker.py

Entire changelog: https://code.google.com/p/web2py/source/list?path=/gluon/portalocker.py

@mdipierro: can you explain why web2py switched back from msvcrt?

[wolph]

Ok... I've had contact with @mdipierro and he wasn't sure why he reverted the code back then... but it's likely that there were problems with it.

So I'm not entirely certain what to do now, switch to the msvcrt code which might be broken, keeping it as it currently is... or try and porting in your (or other) changes.

[techtonik]

On Mon, Mar 24, 2014 at 12:43 AM, Rick van Hattem
notifications@github.comwrote:

Ok... I've had contact with @mdipierro https://github.com/mdipierro and
he wasn't sure why he reverted the code back then... but it's likely that
there were problems with it.

So I'm not entirely certain what to do now, switch to the msvcrt code
which might be broken, keeping it as it currently is... or try and porting
in your (or other) changes.

If there is no evidence that msvcrt code is broken, there is no need to
fear that. I would just try to keep code as close to original and forks as
possible (see Django branch for another fix), so that people could compare
your fork and safely replace their own versions. It will help also to
maintain changelog inside the file. For example:
https://code.google.com/p/chromium/codesearch#chromium/src/courgette/third_party/bsdiff.h
https://code.google.com/p/chromium/codesearch#chromium/src/courgette/third_party/README.chromium

anatoly t.

[wolph]

@techtonik just letting you know, I haven't forgotten about this issue, I guess you're right that merging to a single file can be convenient for people to integrate into their own projects, for maintenance however I prefer to keep the files separate as it improves readability. So I'll see if I can make Travis automatically merge the separate modules to a single module.

I'll also merge the msvcrt code (assuming I can still find it since Google code is down now), since apparently the code is currently already broken on windows :(