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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions src/azure-cli-core/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,16 +57,13 @@
'packaging>=20.9,<22.0',
'paramiko>=2.0.8,<3.0.0',
'pkginfo>=1.5.0.1',
# psutil can't install on cygwin: https://github.com/Azure/azure-cli/issues/9399
'psutil~=5.9; sys_platform != "cygwin"',
'PyJWT>=2.1.0',
'pyopenssl>=17.1.0', # https://github.com/pyca/pyopenssl/pull/612
'requests[socks]'
]

# dependencies for specific OSes
if not sys.platform.startswith('cygwin'):
DEPENDENCIES.append('psutil~=5.9')
Comment on lines -65 to -67
Copy link
Member Author

@jiasli jiasli Mar 16, 2022 •
edited
Loading

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 •
edited
Loading

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



with open('README.rst', 'r', encoding='utf-8') as f:
README = f.read()

Expand Down
6 changes: 2 additions & 4 deletions src/azure-cli/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

azure-cli/src/azure-cli-core/azure/cli/core/auth/persistence.py

Lines 41 to 50 in fb805f4

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

'fabric~=2.4',
'javaproperties~=0.5.1',
'jsondiff~=1.3.0',
Expand All @@ -154,10 +156,6 @@
'xmltodict~=0.12'
]

# On Linux, the distribution (Ubuntu, Debian, etc) and version are checked
if sys.platform == 'linux':
DEPENDENCIES.append('distro')

with open('README.rst', 'r', encoding='utf-8') as f:
README = f.read()

Expand Down