-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[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.
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
Conversation
|
App Configuration |
jsondiff and javaproperties
| 'websocket-client~=0.56.0', | ||
| 'xmltodict~=0.12' | ||
| 'xmltodict~=0.12', | ||
| 'chardet~=3.0.4' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chardetis a requirement ofrequestsfor python2. For python3,chardetdoesn'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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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? 😉
There was a problem hiding this comment.
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.
Description
AppConfig command module introduces three dependencies.
chardet,jsondiffandjavaproperties. Regardless of installation methods, all three should be downloaded during CLI install.Testing Guide
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.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.