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

{Packaging} Use proper PEP 508 environment marker for dependencies #21660

Merged
merged 1 commit into from Mar 17, 2022

Conversation

jiasli
Copy link
Member

@jiasli jiasli commented Mar 16, 2022

Description

Similar to microsoft/knack#257, use proper PEP 508 environment marker for dependencies.

Dependencies added by DEPENDENCIES.append() are only added to the list of requirements if the setup.py is executed. However, azure-cli and azure-cli-core are being distributed as a wheel packages and setup.py is not executed when installing a wheel package. Instead, dependencies are translated into azure_cli-2.34.1-py3-none-any.whl/azure_cli-2.34.1.dist-info/METADATA when building wheels:

Requires-Dist: argcomplete
Requires-Dist: jmespath
Requires-Dist: pygments
Requires-Dist: pyyaml
Requires-Dist: tabulate

What's in the wheel is determined by the packaging system, which is like taking a snapshot when packaging the wheel and the condition evaluation result is frozen into the wheel. Therefore, these conditional install won't work as expected.

conditional install in setup.py -----------> Requires-Dist in whl METADATA -----------> pip
                        frozen by packaging system

@ghost ghost requested review from wangzelin007 and yonzhan March 16, 2022 05:39
@ghost ghost added the Auto-Assign Auto assign by bot label Mar 16, 2022
@ghost ghost assigned jiasli Mar 16, 2022
@ghost ghost added this to the Mar 2022 (2022-04-05) milestone Mar 16, 2022
Comment on lines -65 to -67
# dependencies for specific OSes
if not sys.platform.startswith('cygwin'):
DEPENDENCIES.append('psutil~=5.9')
Copy link
Member Author

@jiasli jiasli Mar 16, 2022

Choose a reason for hiding this comment

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

These lines were added by #15076.

This brought back #9399, making it impossible to install azure-cli on cygwin:

Collecting psutil~=5.9
  Using cached psutil-5.9.0.tar.gz (478 kB)
  Preparing metadata (setup.py) ... error
  error: subprocess-exited-with-error

  × python setup.py egg_info did not run successfully.
  │ exit code: 1
  ╰─> [1 lines of output]
      platform cygwin is not supported
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.

image

Copy link
Member Author

@jiasli jiasli Mar 16, 2022

Choose a reason for hiding this comment

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

This is what it becomes in generated wheel METADATA:

Requires-Dist: psutil (~=5.9) ; sys_platform != "cygwin"

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, anyway, latest cryptography doesn't have cygwin support either: pyca/cryptography#6834

@jiasli jiasli requested a review from niander March 16, 2022 06:13
@@ -139,6 +139,8 @@
'azure-synapse-spark~=0.2.0',
'chardet~=3.0.4',
'colorama~=0.4.4',
# On Linux, the distribution (Ubuntu, Debian, etc) and version are checked for `az feedback`
'distro; sys_platform == "linux"',
Copy link
Member Author

Choose a reason for hiding this comment

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

This is what it becomes in generated wheel METADATA:

Requires-Dist: distro ; sys_platform == "linux"

Copy link
Member

Choose a reason for hiding this comment

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

You should use platform_system == "Linux" .

Here: https://peps.python.org/pep-0508/#environment-markers
It says that sys_platform will be linux in python3 and linux2 in python2

Copy link
Member Author

Choose a reason for hiding this comment

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

It's ok. We have dropped support for Python 2 anyway.

We also use sys.platform elsewhere:

if sys.platform.startswith('win'):
return FilePersistenceWithDataProtection(location)
if sys.platform.startswith('darwin'):
return KeychainPersistence(location, "my_service_name", "my_account_name")
if sys.platform.startswith('linux'):
return LibsecretPersistence(
location,
schema_name="my_schema_name",
attributes={"my_attr1": "foo", "my_attr2": "bar"}
)

Copy link
Member Author

Choose a reason for hiding this comment

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

platform.system() looks weird on cygwin, and sys.platform seems to be easier:

$ python3 -c "import os; print(os.name)"
posix

$ python3 -c "import sys; print(sys.platform)"
cygwin

$ python3 -c "import platform; print(platform.system())"
CYGWIN_NT-10.0-19044

Copy link
Member

Choose a reason for hiding this comment

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

Ok sounds good to me

@yonzhan
Copy link
Collaborator

yonzhan commented Mar 16, 2022

Packaging

@jiasli jiasli merged commit 83c11a4 into Azure:dev Mar 17, 2022
@jiasli jiasli deleted the env-marker branch March 17, 2022 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants