Opened 2 years ago

Closed 12 months ago

#19373 closed Cleanup/optimization (fixed)

Windows file locking requires pywin32

Reported by: techtonik Owned by: marfire
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 2 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design 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 2 years ago by 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 2 years ago by apollo13

  • Triage Stage changed from Design decision needed to Accepted

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

comment:4 Changed 22 months ago by pombredanne

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 18 months ago by marfire

  • Cc k@… added
  • Has patch set
  • Owner changed from nobody to marfire
  • Status changed from new to assigned
  • Version changed from 1.4 to master

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 18 months ago by timo

  • Triage Stage changed from Accepted to Ready 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 16 months ago by akaariai

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 13 months ago by timo

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 13 months ago by Kevin Christopher Henry <k@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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 12 months ago by timo

  • Resolution fixed deleted
  • Severity changed from Normal to Release blocker
  • Status changed from closed to new
  • Version changed from master to 1.7-alpha-2

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

comment:11 Changed 12 months ago by timo

  • Patch needs improvement unset

comment:12 Changed 12 months ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

LGTM

comment:13 Changed 12 months 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 12 months ago by timo

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.
Back to Top