-
Notifications
You must be signed in to change notification settings - Fork 20
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 running btests on Windows #80
Conversation
abd5f37
to
142cfba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must have been pretty darn painful. Just a few quick comments here as you're finalizing this.
754bc09
to
dd84f74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a bit of a pass.
From the naive side: Is there something we fix/work-around in the types involved? Adding non-stdlib dependency when there wasn't one before seems unfortunate to me. |
10005c0
to
2338efc
Compare
I rebased and squashed down all of the fixup commits. I'm down to just two btests failing:
I'll look over the rest of the comments above after I get back from the holiday break. |
2338efc
to
6596525
Compare
6596525
to
17df3c8
Compare
Prior to switching over to the other library, I did attempt to fix the types it was failing on. It turns into a game of whack-a-mole pretty fast, with one type after another failing for varying reasons. I got fairly far into it and then ran into a type that I couldn't figure out how to fix, and figured there had to be a better way to go about it than manually fixing every type and hoping I didn't miss something. |
39e7e54
to
0e4e68d
Compare
096dfca
to
3a224d7
Compare
c56c7bc
to
e56ecd0
Compare
44d2e48
to
66021f1
Compare
If this option isn't here, the Windows runners will reset all of the line endings when it clones to \r\n. This breaks a few of the tests because the comparison will have the wrong line endings.
This is mostly suggestions from pycharm, but also includes some comments from when I was tracing through code.
The reason for this switch is primarily because the stock multiprocessing library has very poor support for pickling of non-primitive types on Windows.
As per the comment, some serializer/unserializers don't produce the identical type when unserializing, failing isinstance().
Using the 'spawn' method for multiprocessing causes the global state to get lost when moving from the parent into the child processes. Rebuilding it by looping over a subset and reinserting them into globals() ensures that they exist.
This changes how runSubprocess works on Windows to insert all of the calls within a temporary bash script. This ensures that the entire environment is available when running the processes, which doesn't work when simply calling subprocess.check_call().
Windows has some issue where `hash()` returns different values for the same string in the different child processes. crc32() returns the same values in each.
This fixes a problem on Windows where multiple TEST-EXEC statements in a test could cause those files to be overwritten by subsequent TEST-EXECs, causing failures.
The original tests.environment btest doesn't work on Windows due to some path differences in the output. This adds a new test that does the same things except does some additional conversions in the test script itself to remove those differences.
66021f1
to
ff90899
Compare
This PR covers the majority of the changes that need to happen to the btest script in order to run tests on Windows. There's a few known issues I'll go into at the end. The big differences here are:
multiprocess
module instead of the built-inmultiprocessing
module, since the default one has some problems with pickling our internal types.mulitprocess
on Windows doesn't seem to like our use of globals here and complains that it can't find objects. The proxied dictionary works around this, and is probably the more correct way of doing this anyways.Known issues:
iosource_mgr
, and that can be used for testing.-j
flag with ctrl-c doesn't work right. There's something different about signal masking on Windows. I spent a day trying to figure this one out and haven't solved it yet.dill
library used bymultiprocess
that changes that internal ID stored for the types.run_cmdseq
previously usedisinstance
to check whether a command being run was aCmdSeq
but because the type IDs are different it was returning false (on all platforms). There's currently a work-around where we just check the type name, but it's likely possible to fix this via implementing custom pickling methods for those types.These changes will require some modifications to the Zeek tree to work right as well, but this PR is independent of those.