Windows MSI (2.0.62) broken

[tjprescott]

Latest MSI is broken. A few things should happen:

• Find root cause and fix prior to upcoming release
• Fix release pipeline to automatically check the MSI release like other channels or add a manual check to install and run az self-test with the MSI.

Workaround

For customers impacted by this bug, you should be able to install the 2.0.61 release with the following link: https://bit.ly/2v9W3id

To Reproduce:

• Install latest MSI
• Run: az self-test
• Result:

D:\github>az self-test
This command has been deprecated and will be removed in a future release.
Running CLI self-test.

Loading all commands and arguments...
Error occurred loading commands!

Retrieving all help...
Help loaded OK.

{'load_commands': ImportError("cannot import name 'StorageAccountTypes'",)}

D:\github>echo %ERRORLEVEL%
1

Environment Summary

Windows-10-10.0.17763-SP0
Python 3.6.6
Shell: cmd.exe

azure-cli                         2.0.62

Extensions:
alias                              0.5.2

Python location 'C:\Program Files (x86)\Microsoft SDKs\Azure\CLI2\python.exe'
Extensions directory 'C:\Users\trpresco\.azure\cliextensions'
Development extension sources:
    D:\github\azure-cli-extensions

Python (Windows) 3.6.6 (v3.6.6:4cf1f54eb7, Jun 27 2018, 02:47:15) [MSC v.1900 32 bit (Intel)]

[marstr]

I got this figured out, and it's an interesting bug.

The problem we're having is because StorageAccountTypes is declared as a copy of DiskStorageAccountTypes here. There's nothing wrong with doing that from the Python SDK's perspective. However, this is tricking some logic that we use while building the Windows Installer.

Here we scrape all of the symbols that AutoRest exports:

azure-cli/build_scripts/windows/scripts/patch_models_v2.py

Lines 87 to 136 in f45a62c

	def solve_one_model(models_module, output_folder):
	"""Will build the compacted models in the output_folder"""
	models_classes = [
	(len(model_class.__mro__), model_name, inspect.getfile(model_class), model_class) for model_name, model_class in vars(models_module).items()
	if model_name[0].isupper() and Model in model_class.__mro__
	]
	# Sort on MRO size first, and then alphabetically
	models_classes.sort(key=lambda x: (x[0], x[1]))
	# Just need the name of exceptions
	exceptions_classes = [
	model_name for model_name, model_class in vars(models_module).items()
	if model_name[0].isupper() and HttpOperationError in model_class.__mro__
	]
	py2_models_classes = [
	(len_mro, model_name, path.replace("_py3.py", ".py"), None)
	for len_mro, model_name, path, _ in models_classes
	]
	paged_models_classes = [
	(model_name, inspect.getfile(model_class), model_class) for model_name, model_class in vars(models_module).items()
	if model_name[0].isupper() and Paged in model_class.__mro__
	]
	enum_models_classes = [
	(model_name, inspect.getfile(model_class), model_class) for model_name, model_class in vars(models_module).items()
	if model_name[0].isupper() and Enum in model_class.__mro__
	]
	if enum_models_classes:
	# Can't be more than one enum file
	enum_file = Path(enum_models_classes[0][1])
	enum_file_module_name = "_"+enum_file.with_suffix('').name
	shutil.copyfile(enum_file, Path(output_folder, as_file_name(enum_file_module_name)))
	else:
	enum_file_module_name = None
	write_model_file(Path(output_folder, as_file_name(MODEL_PY3_NAME)), models_classes)
	write_model_file(Path(output_folder, as_file_name(MODEL_PY2_NAME)), py2_models_classes)
	write_paging_file(Path(output_folder, as_file_name(PAGED_NAME)), paged_models_classes)
	write_complete_init(
	Path(output_folder, "__init__.py"),
	models_classes,
	exceptions_classes,
	paged_models_classes,
	enum_models_classes,
	enum_file_module_name
	)

My first thought when reading through the above was that inspect wasn't finding StorageAccountTypes, due to it being a global variable and not a proper class declaration. However, I debugged locally and discovered that it was in-fact finding StorageAccountTypes. So I had to keep digging, and I found that in code below assumes all enums that were declared were done so in the enums file, not the __init__ file:

azure-cli/build_scripts/windows/scripts/patch_models_v2.py

Line 215 in f45a62c

write_fd.write(f"from .{enum_file_module_name} import {model_name}\n".encode("utf-8"))

Hence, we end up trying to import a symbol from a file where it doesn't exist. There are a few solutions, but if possible, I'd really like to not create any special cases in our code. For that reason, I think the best solution is for me to submit a PR to the azure-sdk-for-python to make the original fix from Laurent Mazuel (@lmazuel) just resemble the code generated by Python a little more.

/cc Laurent Mazuel (@lmazuel), Yugang Wang (@yugangw-msft), Travis Prescott (@tjprescott), Tosin Adewale (@adewaleo)

[lmazuel]

StorageAccountTypes has been renamed to DiskStorageAccountTypes by the Compute team. To anticipate the future, I would strongly suggest you adapt the CLI code anyway, since the next major version I will be able to do will kill StorageAccountTypes compat import

That being said, I'm looking at your PR right now.

[marstr]

As far as I can tell, we do not actually use StorageAccountTypes. It's only that we end up creating an __init__ that's destined to fail.

[adewaleo]

StorageAccountTypes has been renamed to DiskStorageAccountTypes by the Compute team. To anticipate the future, I would strongly suggest you adapt the CLI code anyway, since the next major version I will be able to do will kill StorageAccountTypes compat import

That being said, I'm looking at your PR right now.

Please let us know in the future when you release the compute SDK that breaks StorageAccountTypes.

[lmazuel]

Tosin Adewale (@adewaleo) I would not think like that :). Since you know TODAY, that it will break soon, put a task to do it in the next sprint ;). That would be my advice at least, especially since there is 4 characters to change :)

[adewaleo]

Laurent Mazuel (@lmazuel), sounds good. I was thinking of creating the issue. However, I guess I was unsure because it seems like StorageAccountType is still in use by services such as shared image gallery see this swagger pr and SDK. So it seemed like the compute change isn't complete.