Opened 2 years ago

Closed 16 months ago

#8025 enhancement closed fixed (fixed)

Make Trial work on Windows+Python3

Reported by: hawkowl Owned by: hawkowl
Priority: normal Milestone: Python-3.x
Component: core Keywords:
Cc: Branch: moar-windows-8025-8
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl

Description

To actually move forward, we need Trial to work on Python3 on Windows.

Change History (27)

comment:1 Changed 2 years ago by hawkowl

Author: hawkowl
Branch: branches/moar-windows-8025

(In [45705]) Branching to moar-windows-8025.

comment:2 Changed 2 years ago by hawkowl

Keywords: review added

Okay, builds are green. This makes Trial run, and ports a handful of modules enough to make the test suite not freeze. You can see the Windows 7 Python 3.5 build here: https://buildbot.twistedmatrix.com/builders/windows7-64-py3.5/builds/12 -- it doesn't pass, but it now runs Trial.

comment:3 Changed 2 years ago by hawkowl

Note: The win32api problems are because it doesn't have the needed VC++2015 redistributable installed -- but that will be fixed, and doesn't apply to this test (it just means that the full test suite doesn't run yet).

comment:4 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Many thanks for working on this!

In order to have trial working on py3 on windows, we need twisted.test.test_process on py3 on windows?

I don't know what to say about argv handling in py3 under windows

On py2 I know that stdlib does not properlty handles the arguments... and I am forced to use this helper https://github.com/chevah/compat/blob/master/chevah/compat/nt_unicode_argv.py

I think that we should extend test_process to include a test in which the processs is called with non-ascii arguments.... but maybe we can just add a FIXME comment to have trial workind and continue working in an follow up ticket.

In the same way, we would need a test for running a command located in a non-ascii path and having a non-ascii file name.... and called with non-ascii environment variable key names and values.

At the low level win32process.CreateProcess uses the non wide characters version of CreateProcess so even if we pass wide characters in the right encoding, it will fail. This is a bug/issue with win32api


I don't see changes in the dist3.py ... and the following tests are not already there:

  • twisted.internet.test.test_pollingfile
  • twisted.internet.test.test_win32events

For twisted/internet/win32eventreactor.py maybe we should update its header and associate a test.


Why is asBytesMode mode needed in setContent ? I see that temporarySibling will return the same type as the ext... so maybe we should use _getPathAsSameTypeAs()

I think that the moveTo() method, has the same issue... so it might need a separate ticket.


Please see my comments and re-submit.

Thanks!

comment:5 Changed 2 years ago by hawkowl

Branch: branches/moar-windows-8025branches/moar-windows-8025-2

(In [45737]) Branching to moar-windows-8025-2.

comment:6 Changed 2 years ago by hawkowl

Branch: branches/moar-windows-8025-2branches/moar-windows-8025-3

(In [45808]) Branching to moar-windows-8025-3.

comment:7 Changed 2 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

All green (except for the _winpath twistedchecker failures, because it's glue).

Py3.4, Win64: https://buildbot.twistedmatrix.com/builders/windows7-64-py3.4/builds/13 Py3.5, Win64: https://buildbot.twistedmatrix.com/builders/windows7-64-py3.5/builds/31

Please review.

comment:8 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Hi Hawkie and thanks for the great effort and having trial on Win + py3.

While trying to do the port I assume you hit a lot of problems... I saw that you tried to fix them but I think that all those solved problems should also be documented ... as comments or as tests.


For twisted/internet/win32eventreactor.py maybe we should update its header and associate the test module.


Maybe if not platform.isWindows(): can be rewriten to if platform.isWindows(): and have the comment just after the conditional...rather than in the else branch.

I think that we should create a ticket for symlink on Windows on Py3 and add a FIXME comment each time we add an exception handling code.

What does it mean that the test do not work right?


I see that command line arguments, environment key name and environment values are converted to Unicode.

Do we have tests for calling a Windows process with values which require that conversion?


Do we have a test for the case when util.untilConcludes raises an OSError ?


for reader in list(self._closedAndReading.keys()) is the conversion to list reqired? ... can we just use the iterator?

same for for fd in list(self._writes.keys())

maybe we need a comment to explain why we need to convert them to list.


Please add a comment for the conditional linesep from twisted/logger/test/test_stdlib.py ... I find it hard to understand why it is there.

Same for twisted/test/test_internet.py


For twisted/python/_winpath.py why is Twisted raising C{DeprecationWarning}s without it?

It looks to me like this is an issue related to twisted/python/filepath.py

Can we split it in a separate ticket?


For bytesEnviron ... why not convert them to bytes on Windows?

The new docstring looks very strange to me.

You ask for bytesEnviron... most of the times it returns bytes... but sometimes can also return Unicode. What kind of wizardry is that !?


There are a lot of changes in this patch: twisted/python/filepath.py / twisted/python/lockfile.py / twisted/python/log.py / twisted/python/zippath.py ... without any new tests .

Is that ok?

Can we split those changes into separate tickets, rather than putting them into a single .misc change ?


Please check my comment and resubmit.

Thanks!

comment:9 Changed 2 years ago by hawkowl

Branch: branches/moar-windows-8025-3branches/moar-windows-8025-4

(In [46009]) Branching to moar-windows-8025-4.

comment:10 Changed 2 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Builds are looking reasonable.

See https://buildbot.twistedmatrix.com/boxes-unsupported?branch=/branches/moar-windows-8025-4 for the Windows Py3 builds.

The conditional linesep is weird Python stdlib junk -- I will have to add a comment when I have time :)

With the changes everywhere, without new tests -- the existing tests require these changes to operate, so I don't think they need individual tests.

Please review -- this is getting there, and I won't be able to work on it for a few days, so is a good time for an incrememntal review :)

comment:11 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Great work. Thanks!

Can we convert the removed TODO lines from twisted/internet/win32eventreactor.py into a ticket?


For twisted/test/test_paths.py maybe we should use the same method to detect symlink support as twisted/internet/test/test_address.py .. ie, check for the capability rather than a generic OS name / userAgent sniffing :)

there is also twisted/web/test/test_static.py


For return 10038 can you please put this value into a constat so that it has a human readalbe name?


I think that builds are looking green and changes are not that big :)

the new winpath is a big hack...

Instead of

from ._winpath import mkdir, makedirs, open as _open, fdopen
import twisted.python._winpath as _renameModule

can we just have

from twisted.python import _os_fs

_os_fs.open()
_os_fs.fdopen()
_os_fs.isabs()

It shoudl be much easier to read than _renameModule


Is this ok ? why! :)

from os import mkdir, makedirs, open as _open, fdopen

The change from twisted/python/lockfile.py is dangerous

instead of a simple sleep, can we try to get the size of the new file... and if the size is not ok, sleep more ?... and they retry with a timeout?


Please add in-code comment for the linesep changes.


Please check my comments and resubmit.

Thanks!

comment:12 Changed 2 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Hi Adi, thanks for the review.

  • These TODOs are outdated, and useless -- the IOCP link is the "optimal thing" now, not select().
  • test_address.py doesn't run it on Windows, which is why there's that test against it. It's actually useless, so I've removed the os.symlink check in test_address.py.
  • Made it a constant-y thing.
  • Done.
  • Fixed it up a little bit.
  • Good idea. Done, seems to work fine.
  • Done.

Tests are building, it looks pretty good. Please review.

comment:13 Changed 2 years ago by Adi Roiban

So. What is the status of this branch?

Amber, do you still want to merge it as it is now?

I am trying to help move this forward so here is another review round


for STDLibLogObserverTests.test_basicFormatRendered maybe we should fix the code to have the same results on all supported platforms, rather than updating the test.

Same for twisted/test/test_internet.py reactor.spawnProcess


For symlink can we use os.stat to get file size, rather than opening it and reading its content?


Maybe if hasattr(sys, "exc_clear"): needs a comment.


for twisted/web/test/test_static.py with the change if getattr(os, "symlink", None) is None or platform.isWindows():, No symlink support comment is out of date as even if system supports symlink, the tests are still skipped on Windows.


Can we have this merged with DeprecationWarnings and then fix the _winpath issue in a separate ticket?

Thanks!

comment:14 Changed 2 years ago by hawkowl

Branch: branches/moar-windows-8025-4branches/moar-windows-8025-5

(In [46533]) Branching to moar-windows-8025-5.

comment:15 Changed 2 years ago by hawkowl

Hi Adi,

  • This is a real bug, at least the first bit -- see #8153.
  • I am not sure if that would improve it, since os.stat gets cached sometimes.
  • I'll think about this, I'm not sure if it does -- it's in the rest of the scripts.
  • symlinking doesn't work on Windows period :)
  • I've fixed the issues where the DeprecationWarnings caused actual test failures, and outright removed _winpath.

Builders spun, all green, please review.

comment:16 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Many thanks for your work.

The branch looks better to me :)

Major comments

I think that twisted/python/lockfile.py needs a timeout ... to prevent an infinite loop due to a strange race condition.


In twisted/python/test/test_deprecate.py in WarnAboutFunctionTests.setUp can we have the self.flushWarnings() only called on windows and only for py3 ? and maybe a reference to a ticket which discuss why and how this can be fixed?


While reading the code, it is not clear why we need

if hasattr(sys, "exc_clear"):

or

if hasattr(sys.stdout, "buffer"):

Minor comments

Maybe the changes from twisted/logger/test/test_stdlib.py, twisted/python/log.py and twisted/test/test_internet.py need a reference to the ticket... so that if someone will try to fix that bug will know that the test needs to be updated and will get an idea why we have different behaviour.


For twisted/test/test_paths.py maybe we can move all symlink related tests into a single test case and only call skip once.


Instead of

if getattr(os, "symlink", None) is None or platform.isWindows()

can we have something like

if not platform.hasSymlink()

Please check my comments and resubmit.

Thanks!

comment:17 Changed 2 years ago by hawkowl

Branch: branches/moar-windows-8025-5branches/moar-windows-8025-6

(In [46576]) Branching to moar-windows-8025-6.

comment:18 Changed 2 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Hi Adi, thanks for the review.

  • Done, with tests.
  • Partially done, I need to investigate whether this is even ever fixable. It'll get caught up in the "-Werror" checks, once we get stricter on warnings, though.
  • Added comments.
  • All of these are amended, the root cause was misdiagnosed.
  • I don't think that's a good idea, it's moving it from the area of test to the subject of it... dunno.
  • Done.

Tests are green, including on Py3.4 and Py3.5 on Windows. Please review.

comment:19 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Many thanks for the update. I think that it looks much better.

Major

I see that the new supportsSymlinks is only used in the testing code. Can we start by making it private?

I am not happy about lying about symlink support on newer Windows version. If you have the right permissions... so maybe is best to keep the new helper just for our tests


For twisted/python/lockfile.py why the local import and rename, can we just use a clock and reactor.seconds() so that we can inject rather than patch in tests?

I say that _open is already constructed for monkey patching, but maybe we can break this and use injection.

http://twistedmatrix.com/documents/current/core/development/policy/test-standard.html#real-time


I feel that this branch tries to port to many things at once and changes to many files. Maybe it should be split into dedicated tickets.

here are some suggestions

Process related changes:

  • twisted/internet/_dumbwin32proc.py
  • twisted/internet/_win32stdio.py

Reactor related:

  • twisted/internet/_pollingfile.py
  • twisted/internet/test/_win32ifaces.py
  • twisted/internet/win32eventreactor.py

Path related, filepath, zippath / lockfile changes:

  • twisted/python/filepath.py
  • twisted/python/zippath.py
  • twisted/python/lockfile.py

Final windows py3 migration.

  • the remaining files.

What do you think?

If things go bad, it should be easier to revert just a part of these changes.


Minor

For the sleep with timeout thing I usually start with a small sleep value and then increment it after each run.

In this way, the optimistic approach in which the condition is only slightly delayed should be observed with little extra delay. A simple linear increment factor of 2 should be fine :)


For

      if hasattr(sys, "exc_clear"):
          # exc_clear is only available on Python 2
          sys.exc_clear()

I understand that exc_clear don't exist on Py3 ... but what would happen for py3. Is the exception automatically cleared or we will have to live with 2 different implementations?


Since there are so many changes here and so many unrelated files, if you want to keep this as a single patch I think that it might need a second review for someone who really care about Py3 and Windows.

I am not using py3 and at this time I have little experience with py3 and even less with py3 + windows.


So to keep the quality and scope under control I think that this should be break into dedicated tickets.

Please check my comments and resubmit or request another review from another developers.

Thanks!

comment:20 Changed 2 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Hi Adi,

  • Done, made private.
  • The problem here is that it doesn't use the reactor now, using a Clock would mean that real operation would require a reactor.
  • I don't think it'll help; because without it all, Trial probably won't work great, and we'll get lots of failures once it *does* work.

I've asked Glyph to take a look at this, too :) Builders spun.

comment:21 Changed 2 years ago by Adi Roiban

What is the problem if real operations would require a reactor? ... only the time functionality will be used.

comment:22 Changed 2 years ago by Glyph

Keywords: review removed
Owner: set to hawkowl

Thanks for updating this hawkie.

First I have some optional stuff for you, since some of this might obviate portions of the mandatory stuff:

  1. As adi noted previously, this is (still?) a lot of code, and much of it is very squirrely. If possible, could you split this up and land it separately? Some parts of it are just boring as heck (implements -> implementer for example) and could land with only a cursory glance.
  2. FilesystemLock is a dumb idea. Why do we have synchronous locks? Ugh. Ostensibly, this is in support of DeferredFilesystemLock, which might make sense, if it weren't for the fact that DeferredFilesystemLock calls lock() on the main thread. I am not sure where I'm going with this, except to note that the sleep that polls the filesystem should probably happen off the main thread to avoid actually blocking stuff, and should fail faster if possible.
  3. The write-retrying in twisted.python.log.FileLogObserver seems like a total hack. Long term I feel like that sort of thing ought to be moved to a NoMatterWhatFile that just implements write and flush to do all the horrible best-effort hacks at once, in a way that logger and any legacy stuff in twisted.python.log could share. Maybe even a thing that logger exports as part of its API.
  4. The get-a-byte-stream-no-really code in twisted/test/process_stdinreader.py seems like it might be generally useful; it is reminiscent of twisted.python.compat.ioType. It would be nice for it to live somewhere else since it is definitely not specific to testing process I/O :-).
  5. Rather than make these absolutely heinous py3-specific changes to twisted/python/lockfile.py, you might want to instead switch to using os.open / os.write on both py2 and py3. The whole point of the "c" flag for _fopen is that it makes fflush actually work properly; there's no equivalent _open-level flag because there's no C runtime buffer associated with a file descriptor (as opposed to a FILE*).
  6. Please include Windows version information in the docstring for _supportsSymlinks. I suspect that they are testing the waters with symlink support and might turn it on for regular users at some point, and I'd hate for Twisted's docs to suddenly be wrong about that. (Alternately just refer to Python's docs which already have this called out in the docs for os.symlink).
  7. I guess this is a meta-comment, not about the code itself: if you're going to reply to one of adiroiban's not-numbered-review-points-for-some-reason reviews, consider using Trac's "reply" functionality so that you can quote the points you're responding to and I can correlate them without manually going back and forth? (Also, adiroiban, please consider doing twisted reviews using numbered review points like this one; it has been a convention for a long time and it makes subseqeunt reviews a lot easier to deal with since I can refer back to specific points instead of having to quote your previous review in its entirety if I want to reference something.)

Now for the required stuff:

  1. No newline at end of file on .gitignore :).
  2. The comment in _dumbwin32proc.Process.__init__ says "make sure all arguments are str" but as always that's ambiguous; please always say "unicode" or "text".
  3. There are numerous places throughout this patch where you do if _PY3: if isinstance(something, bytes): something.decode("mbcs"). This strikes me as error-prone and confusing for maintainers. Could you make a (private) maybeMBCS function, where you could hang a single docstring explaining why this is needed? Even though they might only be called once, I would want to factor out a maybeMBCSDict and maybeMBCSList as well just because that'd be easier to read.
  4. twisted/python/lockfile.py
    1. Please use (private) constants for the number of iterations and length of a sleep. In addition to magic numbers being generally bad style, that would enable us to factor out the dissertation on the tradeoffs to a comment around just the two variables, rather than cluttering the middle of the implementation.
    2. The documentation for FilesystemLock.lock says it raises the errors that os.symlink does; that makes RuntimeError an odd choice. Better, perhaps would be an OSError with an ETIME errno or something, so we're adhering to the documentation for the case where lock acquisition might fail.
    3. Don't you want to put the _sleep(0.1) outside the loop, so that it will execute while the file isn't open? If I remember my windowsing right, open in Python will (sometimes?) take an exclusive lock out on the underlying file which prevents other programs from opening it or it from being deleted. Even if not I think it would be clearer.
  5. It seems to me that if bytesEnviron has different defined behavior for py3+windows, then there should be a test for that behavior (raising an exception, say). Except I think there's no reason this function shouldn't do something to help your programs work on Windows; MBCS, say, like all the other stuff dealing with bytes. It's OK for this one change not to get everything perfectly, though; feel free to address this just by filing a follow-up ticket.

Whew. That was tough to review. Just about as tough to write, I must imagine. But I think it's mostly pretty clear why things work the way they do here; most of my feedback is just about pulling things out so it's still clear to read through the abstract algorithms involved here and not have to wade as deeply into platform details. So hopefully we won't have to do too many more rounds...

comment:23 Changed 20 months ago by hawkowl

Splitting this into some other tickets: #8292 for the symlinks stuff, #8291 for implementer changes

comment:24 Changed 20 months ago by hawkowl

Branch: branches/moar-windows-8025-6branches/moar-windows-8025-7

(In [47242]) Branching to moar-windows-8025-7.

comment:25 Changed 20 months ago by hawkowl

https://tm.tl/#8266 should land first so I can meet some of glyph's optional review points, but maybe that's worth another ticket (mostly the iotype stuff).

comment:26 Changed 17 months ago by hawkowl

Branch: branches/moar-windows-8025-7moar-windows-8025-8

comment:27 Changed 16 months ago by Craig Rodrigues

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