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

Why is lxml restricted to specific versions? #659

Closed
tkdchen opened this issue Jan 30, 2022 · 8 comments · Fixed by #738
Closed

Why is lxml restricted to specific versions? #659

tkdchen opened this issue Jan 30, 2022 · 8 comments · Fixed by #738

Comments

@tkdchen
Copy link
Contributor

tkdchen commented Jan 30, 2022

lxml is restricted to <4.7 in commit 5378daa, which tells "Pin lxml dependency to fix unstable CI test runs". I don't find out any usage of lxml in the source code. Looks like this change is for the lxml version issue required by python3-saml in the CI. If it's true, can we move the change to the CI inside .github/ rather than requirements-saml.txt? Please correct me if I misunderstand. Thanks.

@nijel
Copy link
Member

nijel commented Jan 30, 2022

Because of SAML-Toolkits/python3-saml#292

@tkdchen
Copy link
Contributor Author

tkdchen commented Jan 30, 2022

@nijel IIUC, lxml<4.7 is also required for social-core at runtime if that issue is not fixed in python3-saml, do I?

@nijel
Copy link
Member

nijel commented Jan 31, 2022

The issue in python3-saml escalated meanwhile - current versin requires 4.7.0 which is yanked (SAML-Toolkits/python3-saml#297). Let's wait for them to deal with this mess first.

@shanemcd
Copy link

Heads up, it looks like python3-saml has been deprecated: SAML-Toolkits/python3-saml#320

@nijel
Copy link
Member

nijel commented Dec 6, 2022

Switching to https://github.com/IdentityPython/pysaml2 might be an option, help with that is definitely welcome.

This was referenced Dec 6, 2022
@duck-nukem
Copy link
Contributor

duck-nukem commented Jan 3, 2023

Hi, python3-saml has released a new version that changed the required lxml version(s): https://github.com/SAML-Toolkits/python3-saml/blob/master/setup.py#L36 - would this allow social-core to declare the same versions in https://github.com/python-social-auth/social-core/blob/master/requirements-saml.txt#L2 (or use > 4.7.0)?

@nijel
Copy link
Member

nijel commented Jan 4, 2023

I don't think social-core needs any additional restriction. It was added to avoid test breakage in 5378daa. Now it's probably time to completely remove that.

@duck-nukem
Copy link
Contributor

I created a PR with some changes that I thought are sensible - however when I tried to run tests locally using docker-compose run --rm social-tests it seems to run indefinitely with no real progress being made.

@nijel nijel closed this as completed in #738 Jan 6, 2023
nijel added a commit that referenced this issue Jan 6, 2023
* Removes the fixed version for lxml and sets 1.5.0 as the minimum python3-saml version
* Adds explicit command for installing lxml using the no-binary flag
* Adds missing `--force-reinstall` flag to tox.ini

Fixes #659

Co-authored-by: Michal Čihař <michal@cihar.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants