Fix activate bat - Githubissues

qris commented 2 years ago

Checklist

[X] Used a personal fork of the feedstock to propose changes
[X] Bumped the build number (if the version is unchanged)
[ ] Re-rendered with the latest conda-smithy (Use the phrase code>@<space/conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
[ ] Ensured the license file is being packaged.

When both VS2017 Professional and the BuildTools are installed, vswhere returns two lines of output:

>vswhere.exe -nologo -products * -version ^[15.0^,16.0^) -property installationPath
C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional
C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools

These are in random order, and only the last one is used to set _VSINSTALLDIR. If that happens to be the wrong one (Professional instead of BuildTools) then the SDK will not be detected, nothing will be added to the PATH, and compilations will fail with:

'cl.exe' is not recognized as an internal or external command, operable program or batch file.

This change ensures that we always find the BuildTools, even if Professional is also installed.

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

It looks like the 'vc' output doesn't have any tests.
It looks like the 'vs_linux-64' output doesn't have any tests.

qris commented 2 years ago

@conda-forge-admin, please rerender

github-actions[bot] commented 2 years ago

Hi! This is the friendly automated conda-forge-webservice. I tried to rerender for you, but it looks like I wasn't able to push to the fix_activate_bat branch of gamsystematic/vc-feedstock. Did you check the "Allow edits from maintainers" box?

NOTE: PRs from organization accounts cannot be rerendered because of GitHub permissions.

qris commented 2 years ago

@isuruf sure I can do that, but are you sure it's a good idea? In my experience the supplied activate.bat is not compatible with Professional 2017 (perhaps its version of vcvarsall.bat) and fails with this error during environment activation:

%SRC_DIR%>CALL C:\...\conda\conda-bld\...\_build_env\etc\conda\activate.d\vs2017_get_vsinstall_dir.bat 
Windows SDK version found as: "10.0.17763.0"
[ERROR:vcvarsall.bat] Invalid argument found : -vcvars_ver=14.16
[ERROR:~nx0] Error in script usage. The correct usage is:
Syntax:
    vcvarsall.bat [arch] 
  or
    vcvarsall.bat [arch] [version]
  or
    vcvarsall.bat [arch] [platform_type] [version]
where : 
    [arch]: x86 | amd64 | x86_amd64 | x86_arm | x86_arm64 | amd64_x86 | amd64_arm | amd64_arm64
    [platform_type]: {empty} | store | uwp 
    [version] : full Windows 10 SDK number (e.g. 10.0.10240.0) or "8.1" to use the Windows 8.1 SDK.   

The store parameter sets environment variables to support Universal Windows Platform application 
development and is an alias for 'uwp'.

For example:
    vcvarsall.bat x86_amd64
    vcvarsall.bat x86_amd64 10.0.10240.0
    vcvarsall.bat x86_arm uwp 10.0.10240.0
    vcvarsall.bat x86_arm onecore 10.0.10240.0
    vcvarsall.bat x64 8.1
    vcvarsall.bat x64 store 8.1

Please make sure either Visual Studio or C++ Build SKU is installed.

Is there any point in searching for an installation of Professional if it won't work?

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict. Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

It looks like the 'vc' output doesn't have any tests.

isuruf commented 2 years ago

@isuruf sure I can do that, but are you sure it's a good idea? In my experience the supplied activate.bat is not compatible with Professional 2017 (perhaps its version of vcvarsall.bat) and fails with this error during environment activation:

I don't have a Professional 2017 to test, but it certainly works with Community 2017 and your PR would break my setup as it's no longer looking for Community and only looks for BuildTools.

isuruf commented 2 years ago

[ERROR:vcvarsall.bat] Invalid argument found : -vcvars_ver=14.16

~I don't see how you are going to get this error. We are calling vcvars64.bat instead of vcvarsall.bat. Note that vcvarsall.bat doesn't understand-vcvars_verand only onlyvsvars64.bat` does.~

nvm, vcvars64.bat calls vcvarsall.bat

isuruf commented 2 years ago

I still don't understand why professional doesn't work. Have you reported it to Visual Studio team? Can you run vcvars64.bat --help to get the help menu?

qris commented 2 years ago

@isuruf I've excluded just Professional, I hope (I can't build this package yet, so can't test it).

qris commented 2 years ago

Got it! The version of VS2017 Pro that I originally had installed was installationName: VisualStudio/15.2.0+26430.14, and that didn't support the -vcvars_ver parameter:

"C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\VC\Auxiliary\Build\vcvars64.bat" -vcvars_ver=14.16
[ERROR:vcvarsall.bat] Invalid argument found : -vcvars_ver=14.16
[ERROR:~nx0] Error in script usage. The correct usage is:
Syntax:
    vcvarsall.bat [arch]
  or
    vcvarsall.bat [arch] [version]
  or
    vcvarsall.bat [arch] [platform_type] [version]
where :
    [arch]: x86 | amd64 | x86_amd64 | x86_arm | x86_arm64 | amd64_x86 | amd64_arm | amd64_arm64
    [platform_type]: {empty} | store | uwp
    [version] : full Windows 10 SDK number (e.g. 10.0.10240.0) or "8.1" to use the Windows 8.1 SDK.

The store parameter sets environment variables to support Universal Windows Platform application
development and is an alias for 'uwp'.

For example:
    vcvarsall.bat x86_amd64
    vcvarsall.bat x86_amd64 10.0.10240.0
    vcvarsall.bat x86_arm uwp 10.0.10240.0
    vcvarsall.bat x86_arm onecore 10.0.10240.0
    vcvarsall.bat x64 8.1
    vcvarsall.bat x64 store 8.1

Please make sure either Visual Studio or C++ Build SKU is installed.

However our IT team have now upgraded us to installationName: VisualStudio/15.9.43+28307.1778, which does:

"C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\VC\Auxiliary\Build\vcvars64.bat" -vcvars_ver=14.16
**********************************************************************
** Visual Studio 2017 Developer Command Prompt v15.0
** Copyright (c) 2017 Microsoft Corporation
**********************************************************************
[ERROR:team_explorer.bat] Directory not found : "C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\Common7\IDE\CommonExtensions\Microsoft\TeamFoundation\Team Explorer"
[vcvarsall.bat] Environment initialized for: 'x64'

So the answer is to upgrade VS2017 Professional, and we can close this PR :)

conda-forge / vc-feedstock

Fix activate bat #40