Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make $(Architecture) initialization resilient against pre-set values in $env #2842

Open
vatsan-madhavan opened this issue Apr 5, 2020 · 2 comments
Labels
Enhancement Requested Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@vatsan-madhavan
Copy link
Member

See cmderdev/cmder#2290 for details.

I was building dotnet/wpf just now and noticed that it had trouble building correctly.

I debugged it down to these lines that fail to initialize the MSBuild property $(Architecture) correctly (it's initialized to 64 instead of x64), which in turn lead to my (dev) build failures in dotnet/wpf.

    <Architecture Condition="'$(Platform)'=='x64' or '$(Platform)'=='x86' or '$(Platform)'=='arm' or '$(Platform)'=='arm64'">$(Platform)</Architecture>
    <Architecture Condition="'$(Platform)'=='Win32'">x86</Architecture>
    <Architecture Condition="'$(Architecture)'==''">x64</Architecture>

cmder sets $env:architecture to values like 32, 64, which confuses this logic.

https://github.com/cmderdev/cmder/blob/1071221468f2f75f2b63e482deab69c9ca986219/vendor/init.bat#L125-L132

:: Pick right version of clink
if "%PROCESSOR_ARCHITECTURE%"=="x86" (
    set architecture=86
    set architecture_bits=32
) else (
    set architecture=64
    set architecture_bits=64
)

This only affects dev-builds, and only on some alternate-consoles. Still the fix is trivial and we can make this logic a bit resilient.

@miloush
Copy link
Contributor

miloush commented Apr 6, 2020

Wouldn't it be more proper to fix the offending consoles rather than normalizing non-standard practices?

@vatsan-madhavan
Copy link
Member Author

vatsan-madhavan commented Apr 6, 2020

Absolutely! (BTW cmder has already fixed this based on my report - cmderdev/cmder#2294)

That said, it assumes that the offending console is doing something unequivocally wrong. There is no universal compact on the valid values for an environment variable named architecture, unfortunately. For e.g., a console could have set it legitimately to something like amd64, which was quite commonplace before Intel jumped into the game and x64 became the new norm. and $(Architecture)=amd64 would still trigger the same problem on our side.

There is a subtle bug here, which is this: We don't rule out invalid values that we are incapable of dealing with further down the line. I had assumed at the time I built this that $(Architecture) would only be supplied by the build scripts (if they are supplied by an external party at all), and that the values supplied would only be valid ones.

What we need here is a validity check, and possibly also a check for amd64.

    <!-- 
        Architecture is intended to be driven entirely by $(Platform) - clear it first before proceeding
        to infer its value again in case it has been set externally by the environment or supplied as a 
        parameter to MSBuild 
    -->
    <Architecture/>
    <Architecture Condition="'$(Platform)'=='x64' or 
                             '$(Platform)'=='x86' or 
                             '$(Platform)'=='arm' or 
                             '$(Platform)'=='arm64'">$(Platform)</Architecture>

    <Architecture Condition="'$(Platform)'=='Win32' Or 
                             '$(Platform)' == 'AnyCPU' Or 
                             '$(Platform)' == 'Any CPU'">x86</Architecture>
    <Architecture Condition="'$(Platform)'=='amd64'">x64</Architecture>
    <Architecture Condition="'$(Architecture)'==''">x64</Architecture>

    <!-- No suitable value was found - select a default --> 
    <Architecture Condition="'$(Architecture)' !='x86' And 
                             '$(Architecture)' !='x64' And 
                             '$(Architecture)' !='arm' And 
                             '$(Architecture)' !='arm64'">x64</Architecture>

note: In theory, we can't deal with arm or arm64 either yet; I don't expect we'll ever learn to deal with arm(32)..

@fabiant3 fabiant3 added this to the Future milestone May 12, 2020
@fabiant3 fabiant3 added the Enhancement Requested Product code improvement that does NOT require public API changes/additions label May 12, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Enhancement Requested Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

3 participants