[Packaging] Fix #22741: az upgrade: This command becomes non-blocking on Windows
#26464
Conversation
🔄AzureCLI-FullTest
|
ghost
requested review from
|
az upgrade improvement for Windows when Python is upgrade. |
ghost
added
the
| subprocess.Popen(['msiexec.exe', '/i', msi_path]) | ||
| logger.warning("Installation started, please wait for a few minutes.") | ||
| import sys | ||
| sys.exit(0) |
There was a problem hiding this comment.
As stated in the PR description, this makes us be unable to tell if the upgrade is successful. Better to confirm with PM whether this is an acceptable solution. Otherwise, we can develop some "upgrader.exe" written in C.
There was a problem hiding this comment.
We can create a upgrader to update az to prevent running az, but we need to call az to map az upgrade command to that upgrader.exe. We still need to keep az running to retrieve the upgrade status, and we face this issue again.
az upgrade is very complicated. It downloads MSI, waits for installation to finish and updates extensions in one command.
I can hardly come up with an idea to fix this issue without breaking change.
There was a problem hiding this comment.
I thought about it again and feel it is indeed not possible. A GUI application can have such upgrader.exe to upgrade the product itself, but this can't be done for a console application.
Consider this invocation chain: python.exe(1) -> upgrader.exe -> python.exe(2)
- To keep the second
python.execonnected to the original terminal context, every executable on the invocation chain must be alive and wait for its downstream executable to exit. In order words, ifpython.exe(1) subprocessesupgrader.exeand exits, then afterupgrader.exeupgradespython.exe, subprocessespython.exe(2) and exits, it is no longer possible to connectpython.exe(2) back to the original terminal context - the output is out of order. - If the first
python.exeis alive,upgrader.execannot update it.
-> A contradiction!
Consider the following code:
import subprocess
p = subprocess.Popen(['python.exe', '-c', 'import time; time.sleep(3); print("awaken")'])
print('Exiting')Output:
PS D:\cli\testproj> python.exe main.py
Exiting
PS D:\cli\testproj> awaken
|
The first python.exe will exits and gives control back to the terminal. The second python.exe can print, yes, but it is out of order.
How to solve it? Wait for the subprocess to exit:
import subprocess
p = subprocess.Popen(['python.exe', '-c', 'import time; time.sleep(3); print("awaken")'])
p.wait()
print('Exiting')Output:
PS D:\cli\testproj> python.exe main.py
awaken
Exiting
PS D:\cli\testproj> |
There was a problem hiding this comment.
Yes, exactly.
I come up an idea to break the python dependency of python.exe(1) -> upgrader.exe.
We can modify az.cmd, if the param is upgrade, call upgrader.exe, otherwise, run az.
Furthermore, we can write the upgrade logic in bat, it looks feasible. But is it worth writing dedicated logic for Windows?
bebound
changed the title
az upgrade: Fix Python file in use erroraz upgrade: This command is non-blocking on Windows
bebound
changed the title
az upgrade: This command is non-blocking on Windowsaz upgrade: This command becomes non-blocking on Windows
| import sys | ||
| sys.exit(0) | ||
|
|
||
| if exit_code: |
There was a problem hiding this comment.
These below lines can never be reached, right?
There was a problem hiding this comment.
Yes, they are not reachable. This is draft PR for test only currently.
Co-authored-by: Jiashuo Li <[email protected]>
️✔️AzureCLI-BreakingChangeTest
|
| else: | ||
| from azure.cli.core.util import rmtree_with_retry | ||
| logger.warning("Succeeded. Deleting %s", tmp_dir) | ||
| rmtree_with_retry(tmp_dir) |
There was a problem hiding this comment.
I suggest we find a way to preserve this logic, otherwise the temporary MSI will not be deleted.
There was a problem hiding this comment.
The MSI is in the tempfile.mkdtemp() directory (C:\\Users\\xx\\AppData\\Local\\Temp\\tmp1s_cgvau), it appears that windows can delete it automatically.
Manage drive space with Storage Sense
Alternatively, we can save the MSI in .azure or Temp\azure-cli and add logic to clean MSI when run az command.
bebound
modified the milestones:
Backlog,
October 2023 (2023-11-15)❗ Breaking change release ❗
| import subprocess | ||
| exit_code = subprocess.call(['msiexec.exe', '/i', msi_path]) | ||
| # Save MSI to ~\AppData\Local\Temp\azure-cli-msi, clean up the folder first | ||
| msi_dir = Path(tempfile.gettempdir()) / 'azure-cli-msi' |
There was a problem hiding this comment.
Azure CLI doesn't really use pathlib. Consider using os.path to be consistent with the rest of the code.
There was a problem hiding this comment.
pathlib is the recommended way to deal with path operations now.
If you’ve never used this module before or just aren’t sure which class is right for your task, Path is most likely what you need.
For low-level path manipulation on strings, you can also use the os.path module.
--- https://docs.python.org/3/library/pathlib.html
| # Save MSI to ~\AppData\Local\Temp\azure-cli-msi, clean up the folder first | ||
| msi_dir = Path(tempfile.gettempdir()) / 'azure-cli-msi' | ||
| rmtree_with_retry(msi_dir) | ||
| msi_dir.mkdir() |
There was a problem hiding this comment.
Will this line raise exception if rmtree_with_retry can't delete the directory successfully? If so, existing-directory error should be ignored.
There was a problem hiding this comment.
rmtree_with_retry ignores the FileNotFoundError by default, which is my desired behavior.
There was a problem hiding this comment.
rmtree_with_retry also ingores cannot-delete-folder error, and that's why the directory can still exist and cause error to mkdir.
3 participants
Description
Close #22741, close #23947
This PR changes the behavior of
az upgradeon Windows.If the Python version changes in new version, the upgrade is blocked.
(This issue can be reproduced by installing 2.44 and running
az upgrade)In this PR,
az upgradeexits after MSI installation dialog is opened. So the related file is not opened during upgrade.Three breaking changes:
az upgradebecomes non-blocking command, it exit after MSI installation dialog is opened.az upgradeagain to upgrade extensions.Testing Guide
set this in
upgrade_versionThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.