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

Jump to bottom

Removed un-needed python dependencies #5389

Merged
merged 4 commits into from
Mar 5, 2019
Merged

Removed un-needed python dependencies #5389

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e0d755f
Removed un-needed python dependencies
Mike-Dai Mar 4, 2019
d26f026
Add "lxml" and "defusedxml"
Mike-Dai Mar 5, 2019
f6b51c5
Add a comment for XML requirements
Mike-Dai Mar 5, 2019
03dc03f
Merge branch 'master' into issues5101
Mike-Dai Mar 5, 2019
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
12 changes: 6 additions & 6 deletions requirements/pip.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,15 @@ jsonfield==2.0.2
requests==2.21.0
requests-toolbelt==0.9.1
slumber==0.7.1
lxml==4.3.2
defusedxml==0.5.0
Copy link
Member

Choose a reason for hiding this comment

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

I think those are needed when building without using the docker images. We should test that people building without docker can still use their local instance before merging.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like we still need these packages #3082
At least before #5293 is merged

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't care about people building without Docker. It's unsupported.

Copy link
Member

Choose a reason for hiding this comment

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

Have we ever officially documented that we don't support this? I think it's useful for development and much easier for users when they are getting started. Making users install and download docker images adds a huge dependency to people trying to get started.

Copy link
Member

Choose a reason for hiding this comment

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

We only have discussed this, but I agree, It would make harder to start contributing.

Anyway, these libs were needed for api v1.

Copy link
Member

Choose a reason for hiding this comment

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

We didn't document this, but we talked in different opportunities. Most (or all?) of the build changes are tested under the process that uses Docker, so we can't guarantee that local building produces the same output than building with Docker.

pyyaml==3.13
Pygments==2.3.1

# These has to be removed once APIv1 is removed
# https://github.com/rtfd/readthedocs.org/pull/5293
# https://github.com/rtfd/readthedocs.org/pull/3082
lxml==4.3.1
defusedxml==0.5.0

# Basic tools
# Redis 3.x has an incompatible change and fails
# https://stackoverflow.com/questions/53331405/django-compress-error-invalid-input-of-type-cachekey
Expand All @@ -38,9 +42,6 @@ celery==4.1.1 # pyup: ignore

django-allauth==0.39.1

# VCS
httplib2==0.12.1

# GitPython 2.1.11 makes TestGitBackend.test_git_tags to fail because
# of an UnicodeError
# This commit,
Expand All @@ -65,7 +66,6 @@ pyquery==1.4.0
# Utils
django-gravatar2==1.4.2
pytz==2018.9
beautifulsoup4==4.7.1
Unipath==1.1
django-kombu==0.9.4
mock==2.0.0
Expand Down