Skip to content

{Packaging} Remove Windows binary from source repository#12296

Closed
bluca wants to merge 1 commit intoAzure:devfrom
bluca:delete_win_binary
Closed

{Packaging} Remove Windows binary from source repository#12296
bluca wants to merge 1 commit intoAzure:devfrom
bluca:delete_win_binary

Conversation

@bluca
Copy link
Copy Markdown
Member

@bluca bluca commented Feb 20, 2020

Binaries should not be checked in source trees. All
kind of alarms are set off when packaging a new source and
it includes a pre-built binary.

@bluca
Remove Windows binary from source repository
1ba358d 
Binaries should not be checked in source trees. All
kind of alarms are set off when packaging a new source and
it includes a pre-built binary.
@bluca bluca changed the title Remove Windows binary from source repository {Packaging} Remove Windows binary from source repository Feb 20, 2020
@yonzhan
Copy link
Copy Markdown
Collaborator

yonzhan commented Feb 21, 2020

packaging

@bluca
Copy link
Copy Markdown
Member Author

bluca commented Feb 21, 2020

If the CI needs it, then shouldn't it build it?

@jiasli
Copy link
Copy Markdown
Member

jiasli commented Feb 21, 2020

The msi installer of Azure CLI adds the path of az.bat to the PATH environment variable:

azure-cli/build_scripts/windows/Product.wxs

Lines 95 to 101 in 844005a

<Environment Id="AzureCliAddedToPATH"
Name="PATH"
Value="[DynamicCliDir]wbin"
Permanent="no"
Part="first"
Action="set"
System="yes" />

According to Environment Variables doc:

To programmatically add or modify system environment variables, add them to the HKEY_LOCAL_MACHINE\System\CurrentControlSet\Control\Session Manager\Environment registry key, then broadcast a WM_SETTINGCHANGE message with lParam set to the string "Environment". This allows applications, such as the shell, to pick up your updates.

So the installer needs to broadcast the environment variable change using WM_SETTINGCHANGE to shell (explorer.exe). Otherwise the user needs to logout and login for the change to take effect.

To verify that, you may change registry key HKEY_CURRENT_USER\Environment or HKEY_LOCAL_MACHINE\System\CurrentControlSet\Control\Session Manager\Environment to add an environment variable MYENV=somevalue, then open cmd.exe to run echo %MYENV%.

This will not return somevalue because explorer.exe isn't aware of this change. You may end explorer.exe in Task Manager and reboot it then open cmd.exe, now echo %MYENV% shows somevalue. Or you may run propagate_env_change.exe and only reboot cmd.exe, it will also show somevalue. Note that WM_SETTINGCHANGE doesn't affect cmd.exe directly. (Also see chocolatey/choco#1589)

propagate_env_change.exe calls SendMessageTimeoutW with WM_SETTINGCHANGE:

azure-cli/build_scripts/windows/propagate_env_change/main.cpp

Lines 5 to 11 in df3943f

LRESULT dwResult = SendMessageTimeout(HWND_BROADCAST,
WM_SETTINGCHANGE,
0,
(LPARAM)L"Environment",
SMTO_ABORTIFHUNG,
5000,
NULL);

It is included in the msi installer:

azure-cli/build_scripts/windows/Product.wxs

Lines 42 to 53 in 844005a

<!--Custom action to propagate path env variable change-->
<Binary Id="PropagateEnvChangeExe" SourceFile=".\propagate_env_change\propagate_env_change.exe" />
<Property Id="WixQuietExecCmdLine" Value="propagate_env_change.exe"/>
<!--Ignoring the return value if case there is app-hang we can't control -->
<CustomAction Id="PropagateEnvChange"
BinaryKey="PropagateEnvChangeExe"
ExeCommand="propagate_env_change.exe"
Execute="commit"
Return="ignore"/>
<InstallExecuteSequence>
<Custom Action="PropagateEnvChange" Before="InstallFinalize" />
</InstallExecuteSequence>

Removing it causes the CI to report an error. (Though I have no idea why CI still shows green.)

"D:\a\1\s\build_scripts\windows\azure-cli.wixproj" (rebuild target) (1) ->
(Link target) -> 
  D:\a\1\s\build_scripts\windows\Product.wxs(43): error LGHT0103: The system cannot find the file '.\propagate_env_change\propagate_env_change.exe'. [D:\a\1\s\build_scripts\windows\azure-cli.wixproj]

The source code is checked into the repository, but we need a signed binary and it was not worthwhile to do auto code sign just for this little one.

Thanks for understanding.

@jiasli jiasli closed this Feb 21, 2020
@bluca
Copy link
Copy Markdown
Member Author

bluca commented Feb 21, 2020

The source code is checked into the repository, but we need a signed binary and it was not worthwhile to do auto code sign just for this little one.

Please reconsider. I'll have to repack the source downstream to mangle it and remove the offending binary otherwise, and I don't really want to have to do that.
Why does it have to be stored in the git repository? Why can't it be an asset downloaded on demand when needed?

@jiasli
Copy link
Copy Markdown
Member

jiasli commented Feb 21, 2020

Ok I got your point. @fengzhou-msft can we move it to https://azurecliprod.blob.core.windows.net/util/ just like the built-in python?

azure-cli/build_scripts/windows/scripts/build.cmd

Line 15 in 62307e9

set PYTHON_DOWNLOAD_URL="https://azurecliprod.blob.core.windows.net/util/Python366-32.zip"

@bluca
Copy link
Copy Markdown
Member Author

bluca commented Feb 21, 2020

Thank you!

@fengzhou-msft
Copy link
Copy Markdown
Member

fengzhou-msft commented Feb 21, 2020

Ok I got your point. @fengzhou-msft can we move it to https://azurecliprod.blob.core.windows.net/util/ just like the built-in python?

azure-cli/build_scripts/windows/scripts/build.cmd

Line 15 in 62307e9

set PYTHON_DOWNLOAD_URL="https://azurecliprod.blob.core.windows.net/util/Python366-32.zip"

I will work on it. Can you open an issue to track it?

@jiasli jiasli mentioned this pull request Feb 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fengzhou-msft fengzhou-msft Awaiting requested review from fengzhou-msft

@yungezz yungezz Awaiting requested review from yungezz

@haroldrandom haroldrandom Awaiting requested review from haroldrandom

@jiasli jiasli Awaiting requested review from jiasli

Assignees

@fengzhou-msft fengzhou-msft

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bluca @yonzhan @jiasli @fengzhou-msft