Opened 4 years ago

Closed 3 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 Changed 4 years ago by Russell Keith-Magee

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
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 Changed 4 years ago by Michael Manfre

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 Changed 4 years ago by Florian Apolloner

Triage Stage: Design decision neededAccepted

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

comment:4 Changed 3 years ago by Philippe Ombredanne

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 Changed 3 years ago by Kevin Christopher Henry

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 Changed 3 years ago by Tim Graham

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 Changed 3 years ago by Anssi Kääriäinen

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 Changed 3 years ago by Tim Graham

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 Changed 3 years ago by Kevin Christopher Henry <k@…>

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 Changed 3 years ago by Tim Graham

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 Changed 3 years ago by Tim Graham

Patch needs improvement: unset

comment:12 Changed 3 years ago by Aymeric Augustin

Triage Stage: AcceptedReady for checkin

LGTM

comment:13 Changed 3 years ago by Tim Graham <timograham@…>

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 Changed 3 years ago by Tim Graham

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