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

{Package} Migrate CLI packaging system to PEP 420 Implicit Namespace Packages #14372

Merged
merged 78 commits into from
Aug 19, 2020
Merged

{Package} Migrate CLI packaging system to PEP 420 Implicit Namespace Packages #14372

merged 78 commits into from
Aug 19, 2020

Conversation

arrownj
Copy link
Contributor

@arrownj arrownj commented Jul 15, 2020

Description

Close #13293

Background

PEP420 is an official python enhancement proposal which introduces implicit namespace package.

There are 3 ways to declare a namespace package.

  • pkg_resource style
  • pkgutil style
  • native style (PEP420)

Currently, azure cli is using pkg_resource style namespace package, which is no longer recommended per python official document. Besides, pkg_resource style namespace package performs slow when importing python modules.

This PR is to migrate azure cli namespace package to native style. Refer Packaging namespace packages to see details about above 3 namespace package styles.

There is also a guide about how to do azure sdk package. Refer Azure packaging to see the details.

We will follow this guide to to do azure cli packaging.

Goal

The main goal of this task to to remove __init__.py file under azure and cli folder in our final distributions on different platforms, includes:

  • MSI
  • DEB
  • RPM
  • Pypi
  • Homebrew

Changes

In order to achieve this, several changes are needed as below:

azure-cli-dev-tools

  • remove installation of all cli related -nspkg packages(PR)

some python sdk dependency

  • azure-cosmos
  • azure-multiapi-storage

azure-cli

  • remove all -nspkg dependency
  • remove azure-cli-nspkg and azure-cli-command_modules-nspkg
  • update __init__.py, setup.py and MANIFEST.in according to azure packaging guide
  • move __version__ and __author__ to __main__.py
  • update azure_bdist_wheel to fix package __main__.py problem
  • sdk dependency
  • fix test cases

Plan

  1. merge azure-cli-dev-tools PR
  2. build new azdev version
  3. rollback azdev updates in this PR
  4. merge PR

Testing Guide

  • Install azure-cli on different platform, and see whether __init__.py still exists
  • Make sure all previous tests are passed in both current CI pipelines and Feng's new CI pipelines.
  • Make sure .tar.gz and .whl files are same with previous ones.

History Notes

[Component Name 1] BREAKING CHANGE: az command a: Make some customer-facing breaking change.
[Component Name 2] az command b: Add some customer-facing feature.


This checklist is used to make sure that common guidelines for a pull request are followed.

@arrownj arrownj self-assigned this Jul 15, 2020
@arrownj
Copy link
Contributor Author

arrownj commented Jul 15, 2020

PEP420

@arrownj arrownj marked this pull request as ready for review July 21, 2020 05:17
@@ -0,0 +1,12 @@
---
eventgrid partner topic event-subscription update:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in global suppression already.

@@ -0,0 +1,12 @@
#!/usr/bin/env bash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should be deleted as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this file is not referenced anywhere.

@@ -485,7 +485,7 @@ jobs:
displayName: Build Homebrew Formula

dependsOn: BuildPythonWheel
condition: and(succeeded(), in(variables['Build.Reason'], 'IndividualCI', 'BatchedCI', 'Manual'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to build at those trigger condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, will revert these changes before merge it. Just test whether these pipelines can pass.

Copy link
Member

@fengzhou-msft fengzhou-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, please test the upgrade case as well as the new install case.

@zhoxing-ms
Copy link
Contributor

Just be curiously, our relase pipeline also publishes pakage to the following platforms:
ACR, Windows Artifacts, Cloud Shell

Will these changes affect the packages for the above platforms?
If so, have they been tested and evaluated?

@arrownj
Copy link
Contributor Author

arrownj commented Aug 19, 2020

Just be curiously, our relase pipeline also publishes pakage to the following platforms:
ACR, Windows Artifacts, Cloud Shell

Will these changes affect the packages for the above platforms?
If so, have they been tested and evaluated?

We don't consider the scenario like that. We build package on Windows, MAC, pypi, RPM, DEB, the 3 target platforms you mentioned are all using these distributions. We have test it on all OS platforms. So I think it can cover the scenario you mentioned.

@zhoxing-ms
Copy link
Contributor

We don't consider the scenario like that. We build package on Windows, MAC, pypi, RPM, DEB, the 3 target platforms you mentioned are all using these distributions. We have test it on all OS platforms. So I think it can cover the scenario you mentioned.

Got it~ thanks

@@ -24,6 +24,7 @@ def test_config(self):

# C:\Users\{username}\AppData\Local\Temp
tempdir = tempfile.gettempdir()
tempdir = os.path.realpath(tempdir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a comment for this special case on MacOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

Copy link
Member

@jiasli jiasli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My parts LGTM.

@jiasli
Copy link
Member

jiasli commented Dec 27, 2021

This PR was first included in Azure CLI 2.11.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support PEP 420 -- Implicit Namespace Packages
7 participants