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

Azure CLI wheels includes __init__.py files that should not be there #13635

Closed
lmazuel opened this issue May 22, 2020 · 6 comments
Closed

Azure CLI wheels includes __init__.py files that should not be there #13635

lmazuel opened this issue May 22, 2020 · 6 comments
Assignees
Milestone

Comments

@lmazuel
Copy link
Member

lmazuel commented May 22, 2020

A given wheel should only be responsible for one file, and only one. It means no other packages should contain this file too: if two wheels contains the same file, installing them side by side will break the uninstallation scenario.

Concrete example:

  • azure/__init__.py is owned by azure-nspkg, and only this wheel should contain it
  • azure/cli/__init__.py is owned by azure-cli-nspkg, and only this wheel should contain it

This means the current wheel content of a package like azure-cli is wrong, since it contains both copy of these files.
image

To make things worse, the copies of these files use a "pkg_resource" format that is a performance killer. See also #13293

CLI packages MUST NOT contains files that are owned by any nspkg (applies to azure-cli-core, etc.)

@ghost ghost added the needs-triage This is a new issue that needs to be triaged to the appropriate team. label May 22, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented May 22, 2020

@fengzhou-msft please take a look

@ghost ghost removed the needs-triage This is a new issue that needs to be triaged to the appropriate team. label May 22, 2020
@yonzhan yonzhan added this to the S170 milestone May 22, 2020
@lmazuel
Copy link
Member Author

lmazuel commented May 22, 2020

@fengzhou-msft if you're unsure what this means in term of setup.py magic feel free to ping me ;)

@fengzhou-msft
Copy link
Member

@lmazuel For azure/__init__.py, we should use import pkgutil;__path__ = pkgutil.extend_path(__path__, __name__) for development and remove the file in the package. Is that correct? How can we remove it?

@lmazuel
Copy link
Member Author

lmazuel commented Jun 5, 2020

That's correct. You remove those lines from your wheel using this:
https://github.com/Azure/azure-sdk-for-python/blob/7aecf6ffa4f886df25f40cc2cd2ea451dfea7e08/sdk/storage/azure-mgmt-storage/setup.py#L78-L80

See also:
https://github.com/Azure/azure-sdk-for-python/blob/master/doc/dev/packaging.md

@arrownj
Copy link
Contributor

arrownj commented Jun 10, 2020

Hi @lmazuel , as CLI does not support python 2 anymore. So we think we can remove the init.py in both dev and whl environment. Is that right ?

@lmazuel
Copy link
Member Author

lmazuel commented Jun 16, 2020

@arrownj

  • it should not be in the wheel ever, whatever py2 or py3
  • sdist is more interesting, I'm not sure it matters anymore that much if you target just py3 (keep it or leave it, same). Most of all the complex easy-install/dev install/etc. scenarios are gone. If you remove it you should be fine.

@yungezz yungezz modified the milestones: S170, S174 Aug 3, 2020
@yungezz yungezz modified the milestones: S174, S175 - For Ignite Aug 24, 2020
@yungezz yungezz modified the milestones: S175 - For Ignite, S176 Sep 16, 2020
@arrownj arrownj closed this as completed Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants