WIP: CI: Incorporate workaround so as to support 64-bit VS 2008

[jakirkham]

Note: While this can be used by those who have a need to by building a development version of conda-smithy with this support. It will no t be merged. The required functionality is merged into conda-build ( conda/conda-build#861 ) and will make this unnecessary after the next release of conda-build.

Borrowed from @patricksnape's [work](https://github.com/conda-forge/staged-recipes/pull/205/commits/fc99122240b7fef3d03549041d6548488f6a8e1a), this moves the 64-bit support workaround into the conda-smithy. It is integrated into CI in a similar manner as was done for staged-recipes ( https://github.com/conda-forge/staged-recipes/pull/251 ). To get an idea of where this is coming from, this [comment in the Eigen PR](https://github.com/conda-forge/staged-recipes/pull/205#issuecomment-203482768) is particularly informative. As this is done as part of the CI, individual recipes won't need to worry about it.

[jakirkham]

A brief locally test suggest this is being moved into the expected location. Though would definitely appreciate reviews of this.

[jakirkham]

cc @ocefpaf @msarahan @pelson

[ocefpaf]

All those .reg are text right? Why GitHub does think they are binary?

I don't really get all that is being done here, but I trust your judgement and I am 👍

[jakirkham]

All those .reg are text right?

Literally dragged and dropped the vs2008 directory from staged-recipes as I was terrified I would mess one of these up somehow.

Why GitHub does think they are binary?

Not sure, but that is what git thought also and that's how they were in the original PR that @patricksnape gave me.

I don't really get all that is being done here...

Basically, we are convincing Microsoft to use a 64-bit compiler from Window 7 instead of the 32-bit compiler from VS2008 (as it didn't have 64-bit support). That's how @patricksnape described it. Eventually, conda-build will get some logic that will make all of this unnecessary, but that is still a ways off yet.

...but I trust your judgement and I am 👍

Thanks for the vote of confidence. Though I am trusting @patricksnape on this as he seems to know what he is doing on Windows. He has worked on a project with CI automation for builds like conda-forge and used this strategy,

[jakirkham]

If this change is acceptable, it would be nice to have this in a release so I can try it out with this recipe ( conda-forge/staged-recipes#205 ) that needs this fix to build its tests.

[patricksnape]

Huge thanks to @jakirkham for taking the lead on this! - Time differences are a pain and was already offline when this all went down 😄

@ocefpaf I really should have written down everything I learnt when I went through all the pain of setting up our builds over on Menpo. For clarity, the reality of this update is as follows:

When VS 2008 Express was released, there was no 64-bit compiler, because in 2008 a 64-bit compiler was seen as a Professional feature. In 2009, when .NET 3.5 was released (and packaged as the Windows SDK 7), they also shipped the x64 compiler from the PRO version. However, as mentioned from the release notes of the Windows SDK 7:

5.1.6 Windows SDK Configuration Tool does not update Visual Studio 2008 paths for IA64 headers and libraries.

When the Windows SDK Configuration Tool is used to integrate the Windows 7 SDK with Visual Studio 2008, the VC++ Directories for headers and libraries are set to point to the Windows 7 SDK content for x86 and x64 platforms only. Visual Studio will still point to the IA64 headers, libraries and tools that ship with Visual Studio 2008.

To resolve this issue, from Visual Studio 2008, select Tools - Options - Projects and Solutions - C++ Directories. From the ‘Platform’ drop-down, select ‘Itanium’ and from the “Show directories for” drop-down select ‘Include files’. Replace all instances of ‘WindowsSDKDirIA64’ with ‘WindowsSDKDir’. Select ‘Library files’ from the “Show directories for” drop down menu and repeat the previous step.

Unfortunately, the SDK installer was full of bugs, which have never been addressed, presumably because it is so old. For example, various [posts], [have] [addressed] [this]. So this set of fixes essentially applies those that are detailed in the posts above. It is actually even more complicated if you try and cross compile on a 32-bit Windows build for 64-bit. Luckily we don't have to deal with that.

Finally, in terms of the .reg files being seen as binary - that just seems to be Github incorrectly guessing the extension type - you can still click on them and see the text.

[ocefpaf]

Thanks for the explanation @patricksnape!

[jakirkham]

As an experiment, I have re-rendered Eigen with this change, see PR ( conda-forge/eigen-feedstock#1 ). Let's see how it does. If all worked correctly, it should pass. Feel free to wait on merging until we have seen it work there.

[pelson]

I'm pretty much 👍 on this. Of course, it is a fix to an exceptionally ugly problem, so it is never going to be a satisfying PR 😄

Any discussion/issue on this getting into conda-build anytime soon?

[patricksnape]

Yeah will certainly have a look at conda-build sometime soon. As was discussed with @msarahan - we believe that we can fix conda-build to prefer the Python build tools for Windows rather than VS9 - and our gut feeling is that we could sidestep this issue. I am unsure if the Python tools are installed on Appveyor but it is likely that this is a change they would support. However, I have never used the Python build tools so I need to get my head around them first.

[jankatins]

Just FYI (no idea if conda-forge needs more stuff changed than MPL): matplotlib also has such workarounds, but there it looks much shorter: https://github.com/matplotlib/matplotlib/blob/master/appveyor.yml#L63-L68

There is also a bug in conda-build which prevents the usage of the python tools when the normal VC) is installed (both are installed on appveyor): conda/conda-build#729

[patricksnape]

@JanSchulz That could be the case. I believe the registry entries may only be required for other toolsets and it may not be necessary for Appveyor.

Sounds like conda-build just needs to be looked over carefully to fully fix these issues with VS/Python tools.

[jakirkham]

Moral of the story (and this was my reading when it came up in the Eigen PR), the plan is to fix it in conda-build, but we shouldn't expect this fix soon. Also, I think this is likely going to affect any compiled code for Windows. So, Boost, for instance, amongst others will need to be rebuilt.

Not to mention, without this change we are technically shipping 32-bit compiled code and calling it 64-bit. That seems pretty worrisome to me. 😨 Could we maybe accept a little noise so we can do the right thing by our users? Not being a regular Windows user, I am a little concerned about the consequence of installing something that claims 64-bit support, but actually delivers 32-bit support.

If I am misunderstanding anything here, feel free to correct me.

[patricksnape]

@JanSchulz Thanks for the tips, particularly the information in here. I just put a PR up here where I'm attempting to fix these problems. I haven't managed to fully test everything yet but I think maybe this takes a step in the direction of properly setting up the VS environment.

[jakirkham]

@msarahan @pelson @ocefpaf, per our previous discussion I have noted in the OP that this will not be merged and have referenced the relevant merged PR in conda-build that seems to have made this unnecessary going forward. Please feel free to share any thoughts on the wording.

I am considering leaving this open just so that we (or people needing it) will be able to find it and use it themselves. Would that be ok? Certainly it should be closed once there is a conda-build release with the fix. I am just concerned with regards to this interim period. Please let me know your thoughts.

[ocefpaf]

I am considering leaving this open just so that we (or people needing it) will be able to find it and use it themselves.

I am OK using this as a reference point, but I don't think it need to be open. (I don't have any strong feeling about it.)

[jakirkham]

Thanks for the feedback.

I have closed it. I will keep the branch ( jakirkham:ci_vs2008_x64 ) up-to-date with master as I have time. However, I will delete it once there is a conda-build release with the resolution to this problem.

If it would be helpful, I can keep a build of this in my own channel. Alternatively, if people ping me about this issue on a feedstock, I can just apply the fix in a PR.

[ocefpaf]

Thanks for doing this @jakirkham!

[jakirkham]

After doing some testing with @patricksnape per our discussion at today's meeting, it has been determined that conda-build 1.20.1 fixes this problem ( conda-forge/eigen-feedstock#6 ) so no workaround is required.

I am deleting this branch and will no longer support it. Thanks everyone (in particular @patricksnape and @msarahan) for their hard work that has gotten us to this point.

[jakirkham]

Ha, tricked it.

[jakirkham]

This has now been rebased on master.

[jakirkham]

Re-adding to staged-recipes in this PR ( conda-forge/staged-recipes#607 ).

[jakirkham]

While this is re-opened, it seems better to either package this with cmake (as discussed) or possibly as its own package (another idea). That way we aren't letting this clutter everything.

Thoughts on making this a totally separate package, @msarahan?

[msarahan]

I like the idea of a separate package. That minimizes potential screwups from it - we would only install it on appveyor, or point to it if people have setups like appveyor.

[jankatins]

I like the idea of having this as an extra package, which can be installed in non-conda-smithy related repos on appveyor, now that appveyor has conda installed per default.

[jankatins]

Is the "separate package" idea moved to conda-forge/staged-recipes#617 or is that something different?

[jakirkham]

Yeah, that is the idea. Please share your thoughts on that PR.

[pelson]

I think we are suggesting that this shouldn't be part of conda-smithy, but should be packaged in some way if it is still needed. In the interests of reducing redundancy in the feedstocks, that makes an awful lot of sense, so I'm going to go ahead and close this PR.

Thanks!

[jakirkham]

Oh, by no means was I proposing we merge this by leaving it open. It was merely that we discovered these fixes are still required somehow. Thus it was made an action item for me to reopen them.

IMHO would much rather go full speed ahead with PR ( conda-forge/staged-recipes#617 ) and make it a dependency of conda-forge-build-setup so we can leave this ugly mess behind us (hopefully) 😉.

[pelson]

Thanks @jakirkham.