General upgrade of dependencies #3682
Conversation
|
Build failed.
|
AlanCoding
changed the title
|
python websocket-client/websocket-client@e94ed9e Ping @wenottingham this change to BSD, that okay? |
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
|
Build succeeded.
|
|
This seems to have introduced some problems with openssl And trying to install pycurl I'll be looking into that. |
|
Making progress on a few issues here. pycurlThis will not install if you get the new version. The old version will install, and that's why the problem has not been faced before. By pinning the requirement, it seems that we can stabilize this tool again, and I will do that shortly. @rooftopcellist as we discussed, this will restore the tool to functionality for Ansible requirements. Ansible venv python2 vs python3The current instructions do not consider py2 vs py3 in building the Ansible venv. It also seems that the pip compile tool does not handle the switches when making 1 file. Now I have a bit of an answer as to how we can get this back to maintainability - right now, to remake the file as it is, you have to use py2. I tried with py3 and got this diff: diff --git a/requirements/requirements_ansible.txt b/requirements/requirements_ansible.txt
index 5aac69dd26..221acf0f35 100644
--- a/requirements/requirements_ansible.txt
+++ b/requirements/requirements_ansible.txt
@@ -43,18 +43,14 @@ certifi==2019.3.9 # via msrest, requests
cffi==1.12.2 # via bcrypt, cryptography, pynacl
chardet==3.0.4 # via requests
colorama==0.4.1 # via azure-cli-core, knack
-configparser==3.7.4 # via entrypoints
cryptography==2.6.1 # via adal, azure-keyvault, azure-storage, openstacksdk, paramiko, pyopenssl, requests-kerberos, requests-ntlm, secretstorage
decorator==4.4.0 # via dogpile.cache, openstacksdk
docutils==0.14 # via botocore
dogpile.cache==0.7.1 # via openstacksdk
entrypoints==0.3 # via keyring
-enum34==1.1.6; python_version < '3' # via cryptography, knack, msrest, ovirt-engine-sdk-python
-futures==3.2.0; python_version < '3' # via openstacksdk, s3transfer
google-auth==1.6.2
humanfriendly==4.18 # via azure-cli-core
idna==2.8 # via requests
-ipaddress==1.0.22 # via cryptography, openstacksdk
iso8601==0.1.12 # via keystoneauth1, openstacksdk
isodate==0.6.0 # via msrest
jinja2==2.10
@@ -66,7 +62,6 @@ keystoneauth1==3.13.1 # via openstacksdk
knack==0.3.3 # via azure-cli-core
lxml==4.3.3 # via ncclient, pyvmomi
markupsafe==1.1.1 # via jinja2
-monotonic==1.5 # via humanfriendly
msrest==0.4.29
msrestazure==0.4.31
munch==2.3.2 # via openstacksdkso... obviously, we should not be manually trying to decide which libraries are not needed in python3. I will sketch kind of a rough proposal for how we can encode this into the formal process moving forward. @wenottingham I'm looking at #3550 here. I'm not against splitting these out into 2 files in the future (after all, python2 should eventually die), but to keep these changes as minimal as possible, I'd like to see about wrapping up with the single consolidated fine for this development cycle. git dependency resolutionIn jazzband/pip-tools#476, it looks like the problem referenced in this text:
Is resolved. However, again, it's clear that common practice has already been that the git requirements are no longer maintained as part of the dependency tree. Maybe it would be good to add them back in and thus allow the pip-compile tool to handle dependencies such as those. @chrismeyersfsu right now my plan is just to delete all mentions of |
requirements/README.md
Outdated
|
|
||
| * Both pip and setuptools need to be added back into `requirements_ansible.txt` | ||
|
|
||
| * Python3 exceptions need to be re-added back to `requirements_ansible.txt` |
requirements/README.md
Outdated
| If any library has a change to its license with the upgrde, then the license for that library | ||
| inside of `docs/licenses` needs to be updated. | ||
|
|
||
| For certain libraries, a tarball of the library is kept along with the license. |
There was a problem hiding this comment.
For libraries that have source distribution requirements (LGPL as an example), a tarball...
|
New info @ryanpetrello, the pip & setuptools requirements were lost in this change: https://github.com/ansible/awx/pull/3269/files#diff-444e80f30bd2f1e25b779966c9621b9bL112 Every time upgrades are done, it requires manual intervention to maintain these. I've added a blurb to the README to enshrine this in the expected practices. I'm not saying we necessarily want to keep these libraries and this practice. Since they were only just lost in this development cycle, I just have to add them back to be change-adverse. |
requirements/requirements_ansible.in
Outdated
| jinja2==2.10 # required for native jinja2 types for inventory compat mode | ||
| # netconf for network modules | ||
| ncclient==0.6.3 | ||
| # netaddr filter | ||
| netaddr | ||
| # oVirt/RHV | ||
| # oVirt/RHV (see README about pip-compile problem) |
|
Here's the result of doing the 2 different files I mention in the README: You'll notice that this is different from the above diff I shared. I have found this to be (bizarrely) reproducible. It seems that creating new requirements file, as opposed to modify the previously existing python2 file will cause the python3 pip-compile to make different decisions. That might argue in favor of using different requirements files. I know that some of our version-specific requirements come from: The other kind of case (which may become increasingly common) is: https://github.com/mitya57/secretstorage/blob/master/changelog#L33 A library becomes incompatible with python 2. By separating the files, we allow python3 to continue with upgrades, but python 2 declines any version that's not compatible with it. Keeping the single consolidated file, this isn't really possible. I'm just trying to get the facts out there for this decision. |
|
Build succeeded.
|
|
The known item that still needs to be resolved with this is the warning I mentioned above. Here is that warning turned into an error: digging into that. |
| To download the PyPI tarball, you can run this command: | ||
|
|
||
| ``` | ||
| pip download <pypi library name> -d docs/licenses/ --no-binary :all: --no-deps |
There was a problem hiding this comment.
Man, thanks for putting in the legwork to get this process documented better. It needed some love.
|
Verification within this environment: |
|
Well it looks like I could just continue hunting those warnings forever, but only the |
|
Well this seems quite self-explanatory here: https://github.com/twisted/twisted/blob/twisted-19.2.0/src/twisted/python/_setup.py#L95-L97 The versions don't sync up. The dependency resolution should have required that service_identity==18.1.0 or higher was installed. Upgrading I replicated this locally, installed the old Bingo! It picked up the dependencies it should have been picking up all along. So, who's the culprit? Well, we didn't put twisted in our requirements. daphne did. https://github.com/django/daphne/blob/2.3.0/setup.py#L25 Is this daphne's problem? Probably. https://github.com/django/daphne/blob/9c574083ff04037cd728bfda351b5ca3327f1fff/daphne/server.py#L26 These imports, by themselves, are probably enough to require various twisted extras. Those are undeclared extras, and that breaks dependency resolution. This is a very bug-prone practice, and it should be fixed so that we don't face the possibility of regressions in the future with similar dependency upgrades. EDIT: Looks like I resolved this the incorrect way in ce56a5e |
|
Link django/daphne#257 |
|
Build succeeded.
|
|
Build succeeded.
|
|
Due to the upgrades, the status code for an invalid token creation request has changed from 400 to 401. This was an intentional change by the maintainers. |
|
Regarding my previous comment: I believe that diff was still overly-complicated due to some form of human error. Doing some clean runs just now, the diff returned is: This matches exactly with what's expected, and I have gone through the The only py2 vs. py3 differences are where some libraries are exclusive to python2, and should not be installed in python3. Looking at typing for instance, it is a backport of python3 stuff. |
|
Build succeeded.
|
|
Build succeeded.
|
requirements/requirements.txt
Outdated
| @@ -44,79 +44,81 @@ django-taggit==0.22.2 | |||
| django==1.11.20 | |||
| djangorestframework-yaml==1.0.3 | |||
| djangorestframework==3.7.7 | |||
| docutils==0.14 # via python-daemon | |||
There was a problem hiding this comment.
We explicitly removed docutils from this list and put a Makefile directive in here https://github.com/ansible/awx/blob/devel/Makefile#L149 to install docutils. At pip install time a dependency of ansible-runner requires docutils to exist to "build" the package .. it's weird. So we pre-install docutils before installing that ansible-runner dep.
There was a problem hiding this comment.
IIRC, the logic I had for removing the git requirements was that, it turned out that no dependencies pulled it.
Since I originally did that, I've done several rebases of this, running the tool again, and here it is, shows up as a surprise.
It appears that it is listed as a setup_requires, but not a install_requires.
https://pagure.io/python-daemon/blob/master/f/setup.py
I'm still looking into what we can do about this. My current thinking is to change the instructions to say "remove all the git requirements at the end"
There was a problem hiding this comment.
Ok, I removed it and added a note.
I still don't think we need to maintain the -e git+ stuff that we used to have. Maybe we need to add a note to check that no dependencies conflict with the git requirements, but that doesn't mean that we need to throw them into the pot before we cook it.
There was a problem hiding this comment.
Also, I missed this from docutils
# Docutils is only required for building, but Setuptools can't distinguish
# dependencies properly.
# See <URL:https://github.com/pypa/setuptools/issues/457>.
setup_kwargs['install_requires'].append("docutils")So yeah, either we tell pip-compile to ignore libraries like this, or we just let it run and have them deleted at the end. My preference is switching to the latter here.
| kombu==4.2.1 # via asgi-amqp, celery | ||
| lxml==4.3.1 # via xmlsec | ||
| kombu==4.5.0 # via asgi-amqp, celery | ||
| lockfile==0.12.2 # via python-daemon |
There was a problem hiding this comment.
Hmm, maybe I missed this dependency of a dependency. python-daemon version didn't change, but this is being pulled in now
requirements/requirements.txt
Outdated
| pyparsing==2.2.0 | ||
| pyrad==2.1 # via django-radius | ||
| pysocks==1.6.8 # via twilio | ||
| python-daemon==2.2.0 # via ansible-runner | ||
| python-daemon==2.2.0 |
There was a problem hiding this comment.
ansible-runner doesn't pull this in now?
There was a problem hiding this comment.
The automated tool will not add this comment, and I do not know why. Yes, it is listed as a dependency, so I'm not sure what's up with that.
There was a problem hiding this comment.
Got it, that's because it was put into requirements/requirements.in. Since it's a top-level dependency, it is not annotated as being a dependency of another library here.
Here is a change to resolve this, and the output of the tool along with it:
diff --git a/requirements/requirements.in b/requirements/requirements.in
index 05853a486f..747d18b079 100644
--- a/requirements/requirements.in
+++ b/requirements/requirements.in
@@ -35,7 +35,6 @@ psutil==5.4.3
psycopg2==2.7.3.2 # problems with Segmentation faults / wheels on upgrade
pygerduty==0.37.0
pyparsing==2.2.0
-python-daemon==2.2.0
python-dateutil==2.7.2 # contains support for TZINFO= parsing
python-memcached==1.59
python-radius==1.0
diff --git a/requirements/requirements.txt b/requirements/requirements.txt
index 10254944b3..0ae05d9538 100644
--- a/requirements/requirements.txt
+++ b/requirements/requirements.txt
@@ -2,7 +2,7 @@
# This file is autogenerated by pip-compile
# To update, run:
#
-# pip-compile -U -r --allow-unsafe --output-file requirements/requirements.txt requirements/requirements.in
+# pip-compile --allow-unsafe --output-file=requirements/requirements.txt requirements/requirements.in
#
adal==1.2.1 # via msrestazure
amqp==2.4.2 # via kombu
@@ -15,7 +15,7 @@ asn1crypto==0.24.0 # via cryptography
attrs==19.1.0 # via automat, service-identity, twisted
autobahn==19.3.3 # via daphne
automat==0.7.0 # via twisted
-azure-common==1.1.19 # via azure-keyvault
+azure-common==1.1.20 # via azure-keyvault
azure-keyvault==1.1.0
azure-nspkg==3.0.2 # via azure-keyvault
billiard==3.6.0.0 # via celery
@@ -90,7 +90,7 @@ pyopenssl==19.0.0 # via twisted
pyparsing==2.2.0
pyrad==2.1 # via django-radius
pysocks==1.6.8 # via twilio
-python-daemon==2.2.0
+python-daemon==2.2.3 # via ansible-runner
python-dateutil==2.7.2
python-ldap==3.2.0 # via django-auth-ldap
python-memcached==1.59| cffi==1.11.5 # via bcrypt, cryptography, pynacl | ||
| botocore==1.9.23 # via boto3, s3transfer | ||
| cachetools==3.1.0 # via google-auth | ||
| certifi==2019.3.9 # via msrest, requests |
There was a problem hiding this comment.
meh, we are going to explicitly uninstall it anyway.
|
Build succeeded.
|
|
Build succeeded.
|
Update licenses for new versions after dependency upgrades pin pycurl to version that does not break on install implement new workflow for py2/3 requirements management require twisted tls extras, resolve service-identity version Upgrade celery to resolve importlib DeprecationWarning use flags to resolve the unsafe and cache problems
|
Build succeeded.
|
|
Build succeeded (gate pipeline).
|
SUMMARY
This upgrades using the pip-compile dependency resolution. Only very few manually-pinned things are upgraded here, and generally done so to pick up CVE fixes.
ISSUE TYPE
COMPONENT NAME
AWX VERSION
ADDITIONAL INFORMATION
There will be a lot of additional information on this. The upgrade agenda items were the following:
Covering what's listed in:
https://github.com/ansible/awx/network/alerts
kombu
I very specifically want this fix:
celery/kombu#880
Because I believe it'll keep
awx-manage run_dispatcher --statusfrom hangingwhen rabbit becomes unresponsive, and will give a timeout error instead. Version 4.5.0 will pick that up (we could do much lower, if needed).
Django
Old: 1.11.16
New: 1.11.20
Versions .18 and .19 have CVEs the other two are single-item fixes, prevent crashing, fixed packaging. Hopefully this doesn't mess with FIPS???
pyyaml
This had a CVE, and if you upgrade it will start to issue an annoying warning if you don't pass in the loader. The fix was picked up by running the pip tool
cryptography
github suggests cryptography>=2.3
pip-compile gave cryptography==2.6.1
no problem there
paramiko
github suggests paramiko>=2.4.2
pip-compile gave paramiko==2.4.2
no problem there.
related:
#3466