Skip to content

[App Config] Fix dependencies for multiple installations of jsondiff and javaproperties #19792

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

Merged
zhoxing-ms merged 1 commit into from
Oct 7, 2021
Merged

[App Config] Fix dependencies for multiple installations of jsondiff and javaproperties #19792

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
2 changes: 2 additions & 0 deletions src/azure-cli/requirements.py3.Darwin.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,3 +138,5 @@ wcwidth==0.1.7
websocket-client==0.56.0
wrapt==1.11.2
xmltodict==0.12.0
jsondiff==1.2.0
javaproperties==0.5.1
2 changes: 2 additions & 0 deletions src/azure-cli/requirements.py3.Linux.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,3 +139,5 @@ wcwidth==0.1.7
websocket-client==0.56.0
wrapt==1.11.2
xmltodict==0.12.0
jsondiff==1.2.0
javaproperties==0.5.1
2 changes: 2 additions & 0 deletions src/azure-cli/requirements.py3.windows.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,3 +138,5 @@ vsts==0.1.25
wcwidth==0.1.7
websocket-client==0.56.0
xmltodict==0.12.0
jsondiff==1.2.0
javaproperties==0.5.1
7 changes: 4 additions & 3 deletions src/azure-cli/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,8 @@
'azure-synapse-spark~=0.2.0',
'azure-synapse-managedprivateendpoints~=0.3.0',
'fabric~=2.4',
'javaproperties==0.5.1',
'jsondiff==1.2.0',
'javaproperties~=0.5.1',
'jsondiff~=1.2.0',
'packaging~=20.9',
'PyGithub~=1.38',
'PyNaCl~=1.4.0',
Expand All @@ -147,7 +147,8 @@
'semver==2.13.0',
'sshtunnel~=0.1.4',
'websocket-client~=0.56.0',
'xmltodict~=0.12'
'xmltodict~=0.12',
'chardet~=3.0.4'
Copy link
Member

@jiasli jiasli Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

chardet is a requirement of requests 2.25.1 and should be installed automatically:

> pipdeptree -r -p chardet
chardet==4.0.0
  - requests==2.25.1 [requires: chardet>=3.0.2,<5]

https://github.com/psf/requests/blob/v2.25.1/setup.py

requires = [
    'chardet>=3.0.2,<5',

In requests 2.26.0, chardet is not installed anymore:

https://github.com/psf/requests/blob/v2.26.0/setup.py#L45

requires = [
    'charset_normalizer~=2.0.0; python_version >= "3"',
    'chardet>=3.0.2,<5; python_version < "3"',

CLI does use

azure-cli/src/azure-cli-core/setup.py

Line 59 in b84170e

'requests[socks]~=2.25.1',

I tried to pip install azure-cli but couldn't repro. It's unclear to me why chardet is not installed.

Copy link
Member

@jiasli jiasli Oct 8, 2021

Choose a reason for hiding this comment

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

Also, dependencies should be added in alphabetic order.

Copy link
Member

@jiasli jiasli Oct 8, 2021

Choose a reason for hiding this comment

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

It is not a good idea to make the dependency too strict, as it may cause problem in some old Linux distributions for community packagers (#19639).

Copy link
Contributor Author

@shenmuxiaosen shenmuxiaosen Oct 8, 2021

Choose a reason for hiding this comment

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

chardet is a requirement of requests for python2. For python3, chardet doesn't seem to be a dependency. Is it a good practice to loose version in setup.py but keep it in requirements.py3.xxx.txt?

I will send a separate pr to fix the order.

Copy link
Member

@jiasli jiasli Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

chardet is a requirement of requests for python2. For python3, chardet doesn't seem to be a dependency

The link (https://github.com/psf/requests/blob/main/setup.py#L47) you provided is the latest requests main branch.

For requests 2.25.1, chardet is installed even on Python 3. It is from requests 2.26.0 that chardet is no longer installed on Python 3 (#19792 (comment)).

Is it a good practice to loose version in setup.py but keep it in requirements.py3.xxx.txt?

Yes, as setup.py is only used by pip while requirements.py3.xxx.txt is used by our packaging pipeline. requirements.py3.xxx.txt follows the same == pattern as pip freeze.

Copy link
Contributor Author

@shenmuxiaosen shenmuxiaosen Oct 8, 2021

Choose a reason for hiding this comment

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

Then how about we explicitly add chardet in setup.py. Otherwise, it will be accidentally dropped if we upgrade requests to 2.26.0.

Copy link
Member

@jiasli jiasli Oct 9, 2021

Choose a reason for hiding this comment

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

Agreed. This is already merged in this PR, right? 😉

Copy link
Contributor Author

@shenmuxiaosen shenmuxiaosen Oct 11, 2021

Choose a reason for hiding this comment

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

Right, I mean we keep it in setup.py. BTW, here is the pull request to fix dependency order in requriements.py3.xxx.txt.

]

# On Linux, the distribution (Ubuntu, Debian, etc) and version are checked
Expand Down