Opened 11 years ago

Closed 10 years ago

#19373 closed Cleanup/optimization (fixed)

Windows file locking requires pywin32

Reported by: anatoly techtonik Owned by: Kevin Christopher Henry
Component: File uploads/storage Version: 1.7-alpha-2
Severity: Release blocker Keywords:
Cc: k@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I've noticed that file locking logic in django/core/files/locks.py will fail on Windows if no pywin32 is not installed. If this part is critical, ctypes version of portalocker is available from http://roundup.hg.sourceforge.net/hgweb/roundup/roundup/file/tip/roundup/backends/portalocker.py

Change History (14)

comment:1 by Russell Keith-Magee, 11 years ago

Triage Stage: UnreviewedDesign decision needed

PyWin32 isn't listed as an install dependency on Windows, but perhaps it should be.

I'll leave this as DDN because someone who cares/knows about Windows needs to make a call here -- either documenting PyWin32 as a dependency, or finding other options, like portalocker.

comment:2 by Michael Manfre, 11 years ago

I'm not in a position to make a decision, but I'll share some of my insights.

I've needed PyWin32 installed on every windows system because of django-mssql, the file locking support was secondary. The installation is easy and the project doesn't release new version very often.

The only real downside to PyWin32 that I've encountered is that it doesn't play well with virtualenv because --system-site-packages is required. This requirement is not exclusive to PyWin32 and almost every binary package that drops DLLs needs to be installed globally and use --system-site-packages in virtualenvs. This can become a problem with maintaining an automated build system and for some deployment scenarios.

If requiring --system-site-packages is an acceptable, then documenting PyWin32 would be the easiest path forward and I'd be happy to submit a patch. If Django doesn't want to force --system-site-packages, then definitely look for another option that only addresses the file locking issue, instead of all the other stuff that is rolled in to PyWin32.

comment:3 by Florian Apolloner, 11 years ago

Triage Stage: Design decision neededAccepted

We should at least document the gain of an installed PyWin32.

comment:4 by Philippe Ombredanne, 11 years ago

I personally like techtonik proposed solution to use the ctypes port of portalocker he made for Roundup instead of PyWin32.

This could have the benefit --if included in Django -- of not requiring the installation of a third-party package to get the core Django to run, which is the general approach to date. The code is simple enough and no documentation changes would be needed, especially no windows-specific section with added requirements.

Locking is the only area where PyWin32 is needed. Removing that dependency would be a good thing imho.

comment:5 by Kevin Christopher Henry, 11 years ago

Cc: k@… added
Has patch: set
Owner: changed from nobody to Kevin Christopher Henry
Status: newassigned
Version: 1.4master

I definitely think removing the PyWin32 dependency is the way to go, especially since the ctypes version is pretty straightforward.

I've added a patch based on the ctypes port: https://github.com/django/django/pull/1639.

comment:6 by Tim Graham, 11 years ago

Triage Stage: AcceptedReady for checkin

Someone who is more familiar with file locking will have to do a final review, but this looks reasonable to me.

comment:7 by Anssi Kääriäinen, 10 years ago

In don't know who could do the final review here.

Unless it is actually known who should do a final review for this I suggest we merge. Just waiting seems like a bad approach...

comment:8 by Tim Graham, 10 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Ok, I've asked for a rebase of the patch and possibly a mention in the release notes. Will merge it once that's done.

comment:9 by Kevin Christopher Henry <k@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 6fe26bd3ee75a6d804ca3861181ad61b1cca45ea:

Fixed #19373 -- Ported Windows file locking from PyWin32 to ctypes

There wasn't any file locking under Windows unless PyWin32 was
installed. This removes that (undocumented) dependency by using ctypes
instead.

Thanks to Anatoly Techtonik for writing the ctypes port upon which this
is based.

comment:10 by Tim Graham, 10 years ago

Resolution: fixed
Severity: NormalRelease blocker
Status: closednew
Version: master1.7-alpha-2

There's been on regression here on some platforms like App Engine according to this PR.

comment:11 by Tim Graham, 10 years ago

Patch needs improvement: unset

comment:12 by Aymeric Augustin, 10 years ago

Triage Stage: AcceptedReady for checkin

LGTM

comment:13 by Tim Graham <timograham@…>, 10 years ago

In 61fdb8d487f62e5b89b3bef9cb2b212dcec6d1e5:

Fixed regression in file locking on some platforms.

Some platforms with os.name == 'posix' do not have the
fcntl module, e.g. AppEngine.

refs #19373.

comment:14 by Tim Graham, 10 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top