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

Adds Windows support #45

Merged
merged 1 commit into from
Mar 8, 2017
Merged

Adds Windows support #45

merged 1 commit into from
Mar 8, 2017

Conversation

claudiubelu
Copy link
Contributor

dbm_gnu library does not exist on Windows. Uses anydbm on
Windows instead.
Passing preexec_fn to Popen is not supported on Windows. Passes
None instead.
Makes the list command more Windows-friendly on Windows.

@claudiubelu claudiubelu mentioned this pull request Feb 27, 2017
@masayukig
Copy link
Collaborator

oh, cool & awesome!

But, in my environment, the 'test_get_test_run_unexpected_ioerror_errno' fails.
I think there are some issues here.

  1. WindowsError: [Error 5] Access is denied:
    This is caused by chmod(FILE, 0000)[1]
    in test_get_test_run_unexpected_ioerror_errno
    self.assertRaises(IOError, repo.get_test_run, '0')

I'm actually not sure, why this happens..
3. How can we do 'CI'? We found https://tea-ci.org/ but it's not a windows native env but a wine env.. I don't find any other solution so far.

[1]

os.chmod(os.path.join(repo.base, '0'), 0000)

traceback-1: {{{
Traceback (most recent call last):
  File "C:\Users\igawa\stestr\.tox\py27\lib\site-packages\fixtures\fixture.py", line 125, in cleanUp
    return self._cleanups(raise_errors=raise_first)
  File "C:\Users\igawa\stestr\.tox\py27\lib\site-packages\fixtures\callmany.py", line 89, in __call__
    reraise(error[0], error[1], error[2])
  File "C:\Users\igawa\stestr\.tox\py27\lib\site-packages\fixtures\callmany.py", line 83, in __call__
    cleanup(*args, **kwargs)
  File "c:\python27\Lib\shutil.py", line 247, in rmtree
    rmtree(fullname, ignore_errors, onerror)
  File "c:\python27\Lib\shutil.py", line 252, in rmtree
    onerror(os.remove, fullname, sys.exc_info())
  File "c:\python27\Lib\shutil.py", line 250, in rmtree
    os.remove(fullname)
WindowsError: [Error 5] Access is denied: 'c:\\users\\igawa\\appdata\\local\\temp\\tmpzunm83\\.stestr\\0'
}}}

Traceback (most recent call last):
  File "stestr\tests\repository\test_file.py", line 118, in test_get_test_run_unexpected_ioerror_errno
    self.assertRaises(IOError, repo.get_test_run, '0')
  File "C:\Users\igawa\stestr\.tox\py27\lib\site-packages\testtools\testcase.py", line 485, in assertRaises
    self.assertThat(our_callable, matcher)
  File "C:\Users\igawa\stestr\.tox\py27\lib\site-packages\testtools\testcase.py", line 498, in assertThat
    raise mismatch_error
testtools.matchers._impl.MismatchError: <bound method Repository.get_test_run of <stestr.repository.file.Repository object at 0x00000000047C8400>> returned <stestr.repository.file._DiskRun object at 0x00000000047DAEF0>
Ran 48 tests in 13.515s (-4.735s)
FAILED (id=16, failures=1)

@claudiubelu
Copy link
Contributor Author

claudiubelu commented Mar 2, 2017

@masayukig yeah, os.chmod functionality is quite limited in python on Windows. For setting file permissions, there are more native approaches for it.

As far CI goes, I'm planning to use stestr in a Windows CI that will run all the OpenStack oslo projects' unit tests. We could make it have this repo's master branch installed when running the unit tests.

Copy link
Owner

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple questions in line. Not really any blockers though

command = "${PYTHON:-python} -m subunit.run discover -t" \
" %s %s $LISTOPT $IDOPTION" % (top_dir, test_path)

python = 'python' if sys.platform == 'win32' else '${PYTHON:-python}'
Copy link
Owner

Choose a reason for hiding this comment

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

So I get that there isn't env variable replacement on windows, but is the python binary always called 'python'?

I guess it doesn't really matter though

Copy link
Contributor Author

@claudiubelu claudiubelu Mar 2, 2017

Choose a reason for hiding this comment

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

So, this still works, assuming that you have python's dir in the PATH env variable (which is pretty much always).

How you access an environment variable on Windows depends on what you're using: cmd, or powershell: http://pastebin.com/gSh94kw1
The env variable for python python is PYTHONPATH. We could probably have something like this:

if sys.platform != 'win32':
    python = '${PYTHON:-python}'
else:
    python = os.getenv('PYTHONPATH')
...

EDIT: nevermind, PYTHONPATH is not a standard environment variable, I only had it in my work environment.

Copy link
Owner

Choose a reason for hiding this comment

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

Are you sure? I just looked at: https://docs.python.org/2/using/windows.html#excursus-setting-environment-variables and it seems to use it. I'm booting up a windows vm right now so I can test things, because I've got a couple other questions about the dbm stuff that I want to test

from subunit import TestProtocolClient
import subunit.v2
import testtools
from testtools.compat import _b

if sys.platform != 'win32':
from six.moves import dbm_gnu as dbm
else:
Copy link
Owner

Choose a reason for hiding this comment

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

I like this approach. I was trying to solve for the matrix of [python version x operating system] and not having much luck. But I think doing it this way makes sense and just pick the thing that works best for each OS.

# TODO(claudiub): replace import with six.moves.dbm once the dbm move is
# defined: https://github.com/benjaminp/six/issues/179
if six.PY2:
import anydbm as dbm
Copy link
Owner

Choose a reason for hiding this comment

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

So my only concern here is that this uses dbhash by default on python 2 and it doesn't exist at all on python 3. The reason I switched to gdbm is that it existed on both versions of python (and a foolishly assumed all OSes) I'd like to avoid the bug where you have to run with python 3 first is there another dbm type that exists on windows?

Medium term I want to remove the dbm usage because there isn't any reason it has to be dbm. (and long term make the sql repo type the default)

Copy link
Owner

Choose a reason for hiding this comment

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

So I just checked this and the bug I thought was there will hit on windows if you use anydbm. Python2 will use dbhash which no longer exists in python3. (python3 on windows defaults to dbm.dumb) So if you create a repository using python2 first it will crash on python3 when you try to use that same repository.

The only way around this I see is to use dbm.dumb explicitly in both cases. I wonder how big the performance hit is if we used it for every os: https://docs.python.org/3/library/dbm.html#module-dbm.dumb

@mtreinish
Copy link
Owner

This will need to be rebased, we just landed the patch which switches the gdbm usage to use dbm.dumb which should work on windows. So you can drop the bit using anydbm on py2 and dbm on py3 since that should work with what's in tree now. I think this will be good to land after the rebase

@claudiubelu
Copy link
Contributor Author

Great, will do in the morning. :) Thanks!

dbm_gnu library does not exist on Windows. Uses anydbm on
Windows instead.
Passing preexec_fn to Popen is not supported on Windows. Passes
None instead.
Makes the list command more Windows-friendly on Windows.
@claudiubelu
Copy link
Contributor Author

Done.

@mtreinish mtreinish merged commit 2c2fc88 into mtreinish:master Mar 8, 2017
@mtreinish
Copy link
Owner

LGTM, thanks

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.

None yet

3 participants