Windows Support

[aiudirog]

Pull for #32. Currently working on Travis building, no wheels yet. I will squash all of the commits once it's ready.

[aiudirog]

I've squashed all the commits and it's ready for review.

[frozencemetery]

Great! Some initial thoughts (please bear with me, I know very little about Windows):

• I'm hesitant to add more python2 support at this juncture, given the looming deprecation. Do you have an argument in favor of a python2 builder?

• This looks like it's adding support for two different ways to build on Windows; do I have that right? Probably there should be some information in the README about how to do that.

• The <char *> stuff is a bit odd; do you have more information on why that's needed? It seems like it's a problem that maybe we should solve at a different layer.

• How "locked" are Windows python versions? Could we just depend on "3" and get the latest one? (Is there a reason not to just take the latest 3?)

@DirectXMan12 this touches some of your magic deployment goo, so I do want your signoff before merging. (Also with your username, how could I not.)

[aiudirog]

• I would love to drop Python 2 support as it requires extra steps and forced me to go through two more debugging commits. I implemented it because the other OSs had it.

• It can be built 2 ways with 3 compilers. The first is against MIT Kerberos on both the Visual Studio C++ compiler and MinGW. The second is under MSYS against heimdal using MinGW. While MSYS has many less gss features, support was mostly there before I started but conflicted with the regular MinGW support so I cleaned it up. The wheels are built using VS C++, but it is quite easy to change to MinGW (in fact I was forced to for Py2 as it's VS compiler isn't supported anymore).

• gss_OID_desc.elements gss_buffer_desc.value is declared as a void* in the C headers but always treated as a char*. The VS C++ compiler refuses to do pointer arithmetic on void* whereas most compilers simply ignore it GNU compilers have an extension to treat it as a char*. Changing the type in the pxd doesn't work since they don't actually get compiled. If we only allow building on MinGW, the char* casts aren't needed but if people want to build themselves it can get finicky as the VS one is preferred as CPython is built with it. (Edit: Relevant SO)

• Wheels are locked to a major/minor version for C-extensions which is why I chose the latest versions for 3.6 and 3.7. I think we could probably change it to look up the 2 or 3 latest versions and build against those.

[frozencemetery]

I see, the arithmetic occurs because of the way cython handles buf[:len] internally. (You can easily do this without needing pointer arithemtic, and I guess they chose not to.) That's unfortunate, but I think you're right that the char * cast is the right approach.

Will wait for @DirectXMan12's thoughts on the rest.

[aiudirog]

I can also look into opening a pull request with Cython to eliminate the zeros. I'm not thrilled about the char* casts having to be maintained when new extensions have to be added.

Edit: This is the relevant code. It wouldn't be hard to eliminate them, but also isn't the cleanest solution as it kind of just hides the problem.

[aiudirog]

I'm just realizing a made a mistake on my explanation for the char* casts. I was on mobile while refreshing myself on why they were needed and had the wrong struct.

It's gss_buffer_desc.value that I had to cast, not gss_OID_desc.elements. The later does need them but has always had them. I was forced to add them to gss_buffer_desc.value because even though it is declared in cython_types.pxd as a char*, gssapi.h on Windows, Ubuntu, and Arch all declare it as a void*. Since GCC is used most of the time, this may have gone unnoticed.

[frozencemetery]

Interesting. I appreciate you looking into it, but the reality is that we have to support older cython versions anyway, so it wouldn't help much. I still think the casts are the right way to go.

[DirectXMan12]

looked through the build stuff

some of it should probably be pulled out into functions in setup.py -- it's getting long enough that it's harder to read

all the arg splitting needs to be shell-split instead of normal split, most likely

[DirectXMan12]

hey, sorry for all the delay, been swamped with other stuff :-(

[aiudirog]

No problem for the delay.

I took care of those three splits. (Should have done that myself originally, haha)

Overall improvements to setup.py might want to be its own PR. One improvement I had in mind was moving the shared kwargs to Extension into a dictionary or having a dedicated function to avoid repeating them.

[frozencemetery]

@aiudirog If you're willing to make other improvements, feel free to tack them onto this PR as a separate commit - I think that should be fine. (And if you're not, just let us know :) )

[aiudirog]

No problem, I started by abstracting out the Extension initialization and dropping the Py26 support for commands.getoutput.

I'm not quite sure yet of the best way to clean up the rest. Not too much is actually repeated or easily abstracted into useful functions, so maybe the best way would be to have one function for each environment that configures it completely? However that would split up the configuration stages so it wouldn't flow as well top to bottom. Maybe sectioning off each block with clearer/more thorough commenting is the way to go?

Any thoughts?

[frozencemetery]

I guess it's okay to remove 2.6 support - we don't work on el6 without EPEL anymore anyway.

I'm likewise not seeing a better way to handle setup.py. Hoisting a lot of functions out won't help - it'll just get longer and harder to follow, and there's nothing that obviously would benefit from it. The only other way I could see to structure it would be wholly separate buildpaths for each type, but that gets out of hand quickly: we explicitly support six meaningfully different OS/Kerberos/compiler combinations and there's a fair amount of code reuse there. (We probably also work on most of the *BSD, which would make it at least eight if not more, but no one's tested that to my knowledge.)

Will give @DirectXMan12 a couple days to respond, but we can always do a cleanup commit later, so I don't want to hold this up too long just for that.

[aiudirog]

I decided to drop the 2.6 support because it wasn't in the classifiers anyways and you had mentioned dropping 2.7 support earlier.

It looks like there are 4 distinct build paths (Windows, OSX > 10.7, MSYS, & Linux/OSX < 10.7), then the varying support like Heimdal which affects MSYS and Linux. I think I'll give it a try and put it in a separate branch. I'll ping you when it's ready and if we don't like it, we can just throw it away.

[aiudirog]

@frozencemetery Let me know what you think. After testing a few different solutions, I found that having a default config that does most of the heavy lifting and then subclassing it to customize for each platform feels the cleanest. Tested so far on Arch, Windows, and MSYS.