Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Windows #280

Closed
wants to merge 8 commits into from
Closed

Conversation

rmcgibbo
Copy link

I think I still had a problem running asv gh-pages from windows, but this fixes some of the major issues.

@@ -98,7 +98,7 @@ def get_date(self, hash):
def get_hashes_from_range(self, range_spec):
args = ['log', '--quiet', '--first-parent', '--format=format:%H']
if range_spec != "":
args += [range_spec]
args += range_spec.split()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? The range spec is supposed to be a single argument passed to git.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I have been using this to run a single commit with asv, like asv run "-1 <hash>", since git log will take more than one argument.

Copy link
Collaborator

@pv pv Jun 25, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pv
Copy link
Collaborator

pv commented Jun 25, 2015

Does the test suite pass? Did you also check if the peakmem benchmarks work? When I wrote it https://github.com/spacetelescope/asv/blob/master/asv/benchmark.py#L128 worked, but would be useful to check that again now that it has a chance of being actually used.

@pv
Copy link
Collaborator

pv commented Jun 25, 2015

Otherwise, looks great to me. @mdboom: could be useful to use appveyor to run tests on windows

}

while proc.poll() is None:
fds = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are systems that don't have setpgid that are still Unix-like and can still use select. It looks like you've changed the behavior here so anything that doesn't have setpgid doesn't use select (and thus can't provide feedback that the subprocess is running). Can you fix the logic here so we only fall back to proc.communicate on Windows?

@mdboom
Copy link
Collaborator

mdboom commented Jun 25, 2015

I would like to set up Appveyor as part of this PR ideally so that this work doesn't get undone in the future. Thanks for setting this up, @rmcgibbo.

@mdboom mdboom mentioned this pull request Jun 25, 2015
@mdboom
Copy link
Collaborator

mdboom commented Jun 25, 2015

#281 adds Appveyor support. Before merging this PR, we should base it on that one and make sure all the Appveyor tests pass etc.

@rmcgibbo
Copy link
Author

It seems like there is a bug somewhere with the terminal colors using colorama. When I run the tests under python setup.py test, it turns the whole terminal to black text on black backround, and you can't see anything. Commenting out the import colorama; colorama.init() from asv/console.py fixes it. Since py.test also uses colorama, I assume that somehow it's being invoked twice in a way that messes things up.

@rmcgibbo
Copy link
Author

After disabling colorama, here are the results of the test on my windows box. I'm not familiar enough with py.test to know what the error means: https://gist.github.com/rmcgibbo/f053315c620ca9504671

@pv
Copy link
Collaborator

pv commented Jun 25, 2015 via email

proc.kill()
is_timeout = True
else:
try:
if posix:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong indentation (doesn't do what the original code did) -- only the signal handler part should be inside if posix

@pv
Copy link
Collaborator

pv commented Jun 25, 2015

The colorama.init() doesn't really seem callable twice.
I don't see py.test actually calling it. Instead, they have this:
.
colorama = None
try:
import colorama
except ImportError:
pass
...
if hasattr(file, "isatty") and file.isatty() and colorama:
file = colorama.AnsiToWin32(file).stream

@rmcgibbo
Copy link
Author

Okay, I'll try changing it to use the explicit stream

@pv
Copy link
Collaborator

pv commented Jun 26, 2015 via email

@pv pv mentioned this pull request Jun 27, 2015
@pv
Copy link
Collaborator

pv commented Jun 27, 2015

Some extra fixes in gh-282 (separate PR to see if appveyor runs)

@pv
Copy link
Collaborator

pv commented Jun 27, 2015

The py.test INTERNALERROR failure is probably https://bitbucket.org/pytest-dev/py/issue/55/broken-comment-parsing; workaround is to pass --tb=native to pytest

@pv
Copy link
Collaborator

pv commented Jun 27, 2015 via email

@rmcgibbo
Copy link
Author

Using https://github.com/pv/asv/tree/pr-280-windows, I still get black-on-black text in the terminal (powershell or VS command prompt).

@rmcgibbo
Copy link
Author

After disabiling colorama, I do not get an error in test_presence_checks though. That test passed fine.

@pv
Copy link
Collaborator

pv commented Jun 27, 2015

I suppose you don't get test_presence_checks errors even with colorama (redirect the output to file if it's black-on-black)? As said, I didn't manage to reproduce these, but they occur on Appveyor.

For me, the output from pytest is colorless, even when colorama is installed. Colors in asv work fine.

@rmcgibbo
Copy link
Author

With your branch, colorama enabled (redirecting stdout), I get.

platform win32 -- Python 2.7.8 -- py-1.4.30 -- pytest-2.7.2
rootdir: C:\Users\Administrator\asv, inifile:
collected 33 items

test\test_benchmarks.py ...
test\test_compare.py .
test\test_conf.py ..
test\test_dev.py .....
test\test_environment.py X..
test\test_graph.py ....
test\test_machine.py .
test\test_publish.py .
test\test_quickstart.py .
test\test_repo.py .x
test\test_results.py .
test\test_rm.py .
test\test_subprocess.py ..
test\test_util.py .
test\test_web.py s
test\test_workflow.py ....

======== 30 passed, 1 skipped, 1 xfailed, 1 xpassed in 541.50 seconds =========

@pv
Copy link
Collaborator

pv commented Jun 27, 2015

I'll just drop colored printing on windows, not really worth the trouble trying to debug it.

@rmcgibbo
Copy link
Author

+1

@pv
Copy link
Collaborator

pv commented Jun 27, 2015

The "Access is denied" error then seems somehow connected to the setup on Appveyor (I didn't get it with admin user or non-admin users). Not sure what to do about this, I don't know much about windows ACL, so it would be tempting to just mark it as a known failure on windows as it seems it's not encountered in actual use.

@rmcgibbo
Copy link
Author

Given that the current master doesn't work at all on windows, and your PR works on both of our machines, it seems like a clear improvement, even with this appveyor issue.

@pv
Copy link
Collaborator

pv commented Jun 28, 2015

The Appveyor access denied failure appears to be due to Python DLL/exe files inside the environment, probably they have remaining open file handles or something that prevents removal. File removal on NTFS is non-POSIX. Adding sleeps, or using the conda.install.rm_rf implementation for rmtree doesn't seem to help, so this doesn't appear to be an intermittent pending-delete failure that can be solved by retrying the operation. Interestingly, the failure only occurs for the combinations where the Python version asv uses is the same as that used in the environment (2.7 in the test); pytest reports it as xpassed (X) when asv itself runs on 2.6 or 3.4 in the Appveyor tests.

I think this failure is fairly unlikely to occur in real use cases --- first, I and @rcmgibbo do not manage to reproduce it on our machines. Second, environment re-creation is not a common occurrence, and when it happens, the Python interpreter inside the environment is unlikely be running. So I think just marking this as xfail won't be so bad in practice.

@mdboom
Copy link
Collaborator

mdboom commented Jul 6, 2015

Closed by #282.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants