Skip to content
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

Silent failures with invalid arch #27

Closed
PazerOP opened this issue Mar 6, 2021 · 3 comments · Fixed by #28 or OpenDDS/OpenDDS#2473
Closed

Silent failures with invalid arch #27

PazerOP opened this issue Mar 6, 2021 · 3 comments · Fixed by #28 or OpenDDS/OpenDDS#2473

Comments

@PazerOP
Copy link

PazerOP commented Mar 6, 2021

See #26 as well.

If you accidentally provide an invalid arch (like Win32 instead of x86), this action will report success, but will not properly configure the PATH.

For example:

Run ilammy/[email protected]
  with:
    arch: Win32
Found with vswhere: C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Auxiliary\Build\vcvarsall.bat
Configured Developer Command Prompt

Running CMake after this then tries to use GCC.

@ilammy
Copy link
Owner

ilammy commented Mar 7, 2021

Hm... Indeed.

I assumed that vcvarsall.bat likes to pretend that it's successful while not doing anything if the architecture parameter is something that it does not understand. However, it does report the issue. It's this action which does not see the failure and assumes it was successful.

Though, it does seem that vcvarsall.bat does not set the exit code correctly. It just prints

[ERROR:vcvarsall.bat] Invalid argument found : Win32
[ERROR:vcvarsall.bat] Error in script usage. The correct usage is:
Syntax:
    vcvarsall.bat [arch] [platform_type] [winsdk_version] [-vcvars_ver=vc_version] [-vcvars_spectre_libs=spectre_mode]
where :
    [arch]: x86 | amd64 | x86_amd64 | x86_arm | x86_arm64 | amd64_x86 | amd64_arm | amd64_arm64
    [platform_type]: {empty} | store | uwp
    [winsdk_version] : full Windows 10 SDK number (e.g. 10.0.10240.0) or "8.1" to use the Windows 8.1 SDK.
    [vc_version] : {none} for latest installed VC++ compiler toolset |
                   "14.0" for VC++ 2015 Compiler Toolset |
                   "14.xx" for the latest 14.xx.yyyyy toolset installed (e.g. "14.11") |
                   "14.xx.yyyyy" for a specific full version number (e.g. "14.11.25503")
    [spectre_mode] : {none} for libraries without spectre mitigations |
                     "spectre" for libraries with spectre mitigations

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 -vcvars_ver=14.0
    vcvarsall.bat x64 8.1
    vcvarsall.bat x64 store 8.1

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

Then seems to exit successfully, so we extract the (unchanged) environment, and assume it's okay.

I guess I can make this action validate the arch parameter against a list of allowed architectures and fail if it's something weird. (Now I wonder if other parameters should be validated too...)

However, the better way will be to detect that vcvarsall.bat has failed on invalid input.

@ilammy
Copy link
Owner

ilammy commented Mar 7, 2021

I've just published v1.6.0 (aliased as v1) which should react properly to incorrect parameter values and fail this action instead of allowing the build to continue.

Note that arch: Win32 is now not an error as well.

@ilammy
Copy link
Owner

ilammy commented Mar 17, 2021

Since this seems to be working and @PazerOP reacted with “👍”, I guess we're done here.

@ilammy ilammy closed this as completed Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants