Attempting to improve the logic for VS on Win

[patricksnape]

Both VS9 and VS10 do not work out of the box for 64-bit builds.
This should try and remedy that. For VS9 it prefers the
Microsoft Visual C++ Compiler for Python 2.7 and when
falling back VS9 will call the correct bat file manually, rather
than relying on the broken logic in vcvarsall.bat.

For VS10, it will correctly call the Windows SDK rather than
relying on the also potentially incorrect vcvarsall.bat.

This was pretty difficult to test in my VM because I broke
it trying to test the different combinations. I need to create
a clean image and try each of the scenarios.

What would be the best way to test this? I guess builds
need to be kicked off on Appveyor?

[jakirkham]

@msarahan, looks like there is a PR for straightening this out. 😉

[msarahan]

This is excellent work @patricksnape

Testing this would be hard without something like vagrant + salt. I think an adequate substitute might be something like this pseudocode:

def setUp():
    for file in [all known vars bat files]:
        if exist file:
             rename file to .bak

def test_vs2008_no_vc_for_python():
    create bat file where vs 2008 expects it.  contents can be just "exit 0"
    create bat file where vc_for_python expects it.  Contents should be "exit 1"

(more tests, but alter the exit codes appropriately so that finding the wrong one raises a subprocess error)

def tearDown():
    restore .bak files to .bat files

I can put that together later tonight - or if you want to pick it up, that works too. Post here if you want to work on that, so I don't duplicate your effort.

[jakirkham]

Given the timezone difference, he might already be asleep.

[msarahan]

Sure. Any comment on the test strategy I proposed, @jakirkham?

[msarahan]

Tests being done at patricksnape#1

[patricksnape]

Sorry, was out of action as @jakirkham correctly guessed. Merged @msarahan's awesome testing suite to keep the action over here. As previously mentioned, would be really good to make 100% this still works before merging!

[msarahan]

No problem. Github is not realtime communication. I just wanted to make sure to not duplicate effort.

To get appveyor going on this, we need to set up appveyor here (conda-build). This is long overdue. I'll see what I can do about it.

I'm a little skeptical of my test suite though. I realized that I had my logic inverted here: https://github.com/conda/conda-build/pull/861/files#diff-910f3184f6aa73a494bd48608b5a8bf7R43

anything that is not in good locations should error out. At least some of the tests should have been failing. When I flip that logic, the tests still all pass. I thought it might be the calling syntax, but this checks out OK:

C:\conda-build>echo exit /b 1 > test1.bat

C:\conda-build>cmd /c test1.bat

C:\conda-build>exit /b 1

C:\conda-build>echo %ERRORLEVEL%
1

I changed to exit /b 1 so that it didn't kill my prompt.

Any ideas on a better test here? Any suggestions on how i'm ending up always passing?

[msarahan]

I'm working on Appveyor restoration here: #864

If you pull in that branch, you might be able to utilize it before that PR gets merged.

[msarahan]

#864 is merged. I'll keep playing with the tests - I'm now also testing the tests to make sure we aren't getting meaningless passes.

[msarahan]

@patricksnape the tests are now "fixed" - but it warrants a little further discussion. The tests are a little dumb: they lack some of the encoding about when it's OK to have one but not the other. For example, the VC for Python overrides Visual Studio, but that override is a failing test right now, because the tests are testing that a particular environment gets activated when all environments are present. Is this a good way to do tests? If so, we can correct the code by making it do more try/except stuff, actually trying to run scripts rather than just verifying that they exist.

[patricksnape]

OK I had a look, agree with most of the changes. I can see where you are coming from w.r.t the VS 2008 Express changes - however, if we don't want to explicitly perform that, I believe we should maybe print a warning at least? Or I fear you will get a lot of people who will run into the error and start raising bugs - maybe we write it in the Wiki and then somehow direct people there? Or maybe when we detect a setup error in Windows at all we try and route them to the Wiki where we can answer many questions?

[patricksnape]

Before merging this, one thing I would like to do is set up a dummy CMake project and make sure that CMake finds the correct VS on Appveyor in each case and is actually able to build. For example, the VS 2008 workaround I have has not been tested though logically it should work.

[msarahan]

Great. That sounds like a good additional test. Ping me when you're ready. IMHO this is ready to merge now, but that will further improve this PR.

[msarahan]

I guess the harder part about that is that you actually have to have all the compilers installed to test them. So, it'll be valuable on AppVeyor, but I'd like to limit that test to being something that we manually enable on the command line, so that it doesn't trip up testing on other, less complete build platforms (like my computer)

[patricksnape]

OK, that took me longer than I would have liked. Here are the things that I've learnt:

• vcvarsall.bat will never EXIT 1. If the given configuration, say amd64, does not exist than it just echos an error and EXIT 0. This means we can't check for these kinds of failures using if errorlevel 1.
  • The Windows SDK SetEnv.cmd also never EXIT 1.
  • The Windows SDK explicitly lives in Program Files rather than Program Files (x86)
  • CMake generators that specify VS versions explicitly look for either msbuild or devenv. This means they don't work with the Python C++ Compiler. However, the NMake generators look for cl and so they do work.

In short, It looks like a bunch of changes but a lot of it was just cleaning up - the list of changes is:

• Fix a bug whereby we weren't looking for the ALL_USERS (the Common Files one) installed Python C++ Compiler in the correct place. Needed to use %PROGRAMFILES% but we were using %LOCALAPPDATA%.
• Always call the Windows SDK for Visual Studio 2010/Python 3.4. In the case where it isn't installed, it will fail, but then we also always call the Visual Studio 2010 vcvarsall. We could possibly change this to and if errorlevel 1. Therefore, remove the Windows SDK from the test faking.

Finally, make sure that this actually all works using this test harness which Appveyor builds here - which now successfully build. As a note, this just tests that it successfully builds - I am just manually checking the correct compiler at the moment from the NMake output. Also, I am not checking the Common Files Python C++ Compiler location as it doesn't exist on Appveyor.

Therefore, I think I am now happy with this, barring the possible if errorlevel 1 for calling VS2010. Also, you can cancel all the builds up to the last one to speed Appveyor up. One last thing is that this took a bit of messing around with, so I could squash the last few commits if you like.

[msarahan]

Time well spent! That's a lot of valuable information, and a lot of bad assumptions on my part. Thanks for digging in. I'll merge once appveyor finishes.

[msarahan]

Squash if you want, but it's fine as is.

[msarahan]

Thanks @patricksnape, this was a bad bug, and you have greatly improved this code.

[patricksnape]

I added a note about the NMake/VS9 with Python tools to the Wiki.

[jakirkham]

Sure. Any comment on the test strategy I proposed, @jakirkham?

Sorry, I missed this comment, @msarahan. I think the test strategy sounds reasonable and agree.

Thanks so much @patricksnape and @msarahan for working on this and getting it integrated into conda-build. 😄

[rvianello]

thanks!