[Django] #19373: Windows file locking requires pywin32
Django
--------------------------------------+--------------------
Reporter: techtonik | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: File uploads/storage | Version: 1.4
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
I've noticed that file locking logic in
[https://github.com/django/django/blob/master/django/core/files/locks.py
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
--
Ticket URL: <https://code.djangoproject.com/ticket/19373>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: techtonik | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.4
Component: File | Resolution:
uploads/storage | Triage Stage: Design
Severity: Normal | decision needed
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by russellm):
* needs_better_patch: => 0
* stage: Unreviewed => Design decision needed
* needs_tests: => 0
* needs_docs: => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19373#comment:1>
Django
Reporter: techtonik | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.4
Component: File | Resolution:
uploads/storage | Triage Stage: Design
Severity: Normal | decision needed
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19373#comment:2>
Django
Reporter: techtonik | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: File uploads/storage | Version: 1.4
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by apollo13):
* stage: Design decision needed => Accepted
Comment:
We should at least document the gain of an installed PyWin32.
--
Ticket URL: <https://code.djangoproject.com/ticket/19373#comment:3>
Django
Reporter: techtonik | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: File uploads/storage | Version: 1.4
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19373#comment:4>
Django
Reporter: techtonik | Owner: marfire
Type: Cleanup/optimization | Status: assigned
Component: File uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by marfire):
* status: new => assigned
* cc: k@… (added)
* has_patch: 0 => 1
* version: 1.4 => master
* owner: nobody => marfire
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19373#comment:5>
Django
Reporter: techtonik | Owner: marfire
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: File | Resolution:
uploads/storage | Triage Stage: Ready for
Severity: Normal | checkin
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
Comment:
Someone who is more familiar with file locking will have to do a final
review, but this looks reasonable to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/19373#comment:6>
Django
Reporter: techtonik | Owner: marfire
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: File | Resolution:
uploads/storage | Triage Stage: Ready for
Severity: Normal | checkin
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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...
--
Ticket URL: <https://code.djangoproject.com/ticket/19373#comment:7>
Django
Reporter: techtonik | Owner: marfire
Type: Cleanup/optimization | Status: assigned
Component: File uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19373#comment:8>
Django
Reporter: techtonik | Owner: marfire
Component: File uploads/storage | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Kevin Christopher Henry <k@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"6fe26bd3ee75a6d804ca3861181ad61b1cca45ea"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19373#comment:9>
Django
Reporter: techtonik | Owner: marfire
Type: | Status: new
Cleanup/optimization | Version:
Component: File | 1.7-alpha-2
uploads/storage | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: closed => new
* version: master => 1.7-alpha-2
* resolution: fixed =>
* severity: Normal => Release blocker
Comment:
There's been on regression here on some platforms like App Engine
according to this [https://github.com/django/django/pull/2436 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/19373#comment:10>
Django
Reporter: techtonik | Owner: marfire
Type: | Status: new
Cleanup/optimization | Version:
Component: File | 1.7-alpha-2
uploads/storage | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* needs_better_patch: 1 => 0
Comment:
[https://github.com/django/django/pull/2442 Updated PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/19373#comment:11>
Django
Reporter: techtonik | Owner: marfire
Type: | Status: new
Cleanup/optimization | Version:
Component: File | 1.7-alpha-2
uploads/storage | Resolution:
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* stage: Accepted => Ready for checkin
Comment:
LGTM
--
Ticket URL: <https://code.djangoproject.com/ticket/19373#comment:12>
Django
Reporter: techtonik | Owner: marfire
Type: | Status: new
Cleanup/optimization | Version:
Component: File | 1.7-alpha-2
uploads/storage | Resolution:
Severity: Release blocker | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"61fdb8d487f62e5b89b3bef9cb2b212dcec6d1e5"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19373#comment:13>
Django
Reporter: techtonik | Owner: marfire
Cleanup/optimization | Version:
Component: File | 1.7-alpha-2
Severity: Release blocker | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/19373#comment:14>