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

[build] Use pip to install setup.py dependency instead of python setup.py install #8997

Merged
merged 4 commits into from
Oct 27, 2021

Conversation

qiluo-msft
Copy link
Collaborator

@qiluo-msft qiluo-msft commented Oct 18, 2021

Why I did it

Fix a recent build error introduced by a pre-release redis-py. This is a general issue because python setup.py install (ie easy_instal) does not ignore pre-release versions. The fix is suggested by pypa/setuptools#855 (comment)

[ building ] [ target/python-wheels/redis_dump_load-1.1-py2-none-any.whl ] 
Applying patch ../redis-dump-load.patch/0001-Use-pipelines-when-dumping-52.patch
patching file redisdl.py

Applying patch ../redis-dump-load.patch/0002-Fix-setup.py-for-test-and-bdist_wheel.patch
patching file setup.py

Now at patch ../redis-dump-load.patch/0002-Fix-setup.py-for-test-and-bdist_wheel.patch
[ finished ] [ target/python-wheels/redis_dump_load-1.1-py2-none-any.whl ] 
[ FAIL LOG START ] [ target/python-wheels/redis_dump_load-1.1-py2-none-any.whl ]
[ REASON ] :      target/python-wheels/redis_dump_load-1.1-py2-none-any.whl does not exist  
[ FLAGS  FILE    ] : [] 
[ FLAGS  DEPENDS ] : [broadcom] 
[ FLAGS  DIFF    ] : [broadcom ] 
/sonic/src/redis-dump-load /sonic
running test
Searching for redis
Reading https://pypi.org/simple/redis/
Downloading https://files.pythonhosted.org/packages/62/ab/6491b41bbfb938afbc4424164983d1def3c59434c77e8cf710213be03fed/redis-4.0.0b1.tar.gz#sha256=f778e27d542ba1f43a6b02a80fa904d8a49e5d3b824ec5fb3f0d5cbdba11e4cd
Best match: redis 4.0.0b1
Processing redis-4.0.0b1.tar.gz
Writing /tmp/easy_install-ZCmBMQ/redis-4.0.0b1/setup.cfg
Running redis-4.0.0b1/setup.py -q bdist_egg --dist-dir /tmp/easy_install-ZCmBMQ/redis-4.0.0b1/egg-dist-tmp-QcQyOe
Traceback (most recent call last):
  File "setup.py", line 41, in <module>
    'Topic :: System :: Archiving',
  File "/usr/lib/python2.7/dist-packages/setuptools/__init__.py", line 145, in setup
    return distutils.core.setup(**attrs)
  File "/usr/lib/python2.7/distutils/core.py", line 151, in setup
    dist.run_commands()
  File "/usr/lib/python2.7/distutils/dist.py", line 953, in run_commands
    self.run_command(cmd)

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

xumia
xumia previously approved these changes Oct 18, 2021
@xumia
Copy link
Collaborator

xumia commented Oct 18, 2021

@qiluo-msft , we need to add the labels, right?

@fastiuk
Copy link
Contributor

fastiuk commented Oct 18, 2021

In my opinion, it is better to restrict the required version of redis of redis-dump-load module.
It will eliminate all future problems.
src/redis-dump-load/requirements.txt:

diff --git a/requirements.txt b/requirements.txt
index 7800f0f..3fc0632 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1 +1 @@
-redis
+redis==3.5.3

@MalteJ
Copy link

MalteJ commented Oct 18, 2021

The problem is that redis client v4 will drop support for Python2 (see redis/redis-py#1318)
So there are two things we should do:

  1. Pin the redis client to version 3.5.3 as described by @fastiuk
  2. Remove Python 2 completely from SONiC

@qiluo-msft qiluo-msft mentioned this pull request Oct 18, 2021
5 tasks
@qiluo-msft qiluo-msft marked this pull request as draft October 19, 2021 03:59
@qiluo-msft qiluo-msft marked this pull request as ready for review October 22, 2021 09:26
@qiluo-msft
Copy link
Collaborator Author

@saiarcot895 I revert your fix and redo the fix. Could you please help review?

@@ -643,6 +643,8 @@ $(addprefix $(PYTHON_WHEELS_PATH)/, $(SONIC_PYTHON_WHEELS)) : $(PYTHON_WHEELS_PA
pushd $($*_SRC_PATH) $(LOG_SIMPLE)
# apply series of patches if exist
if [ -f ../$(notdir $($*_SRC_PATH)).patch/series ]; then QUILT_PATCHES=../$(notdir $($*_SRC_PATH)).patch quilt push -a; fi
# Use pip instead of later setup.py to install dependencies into user home, but uninstall self
pip$($*_PYTHON_VERSION) install . && pip$($*_PYTHON_VERSION) uninstall --yes `python setup.py --name`
Copy link
Contributor

Choose a reason for hiding this comment

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

Does pip read the requirements.txt file by default for getting the list of packages to install?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What you mentioned is pip -r requirements.txt. And pip install . is checking the setup.py for dependencies, install them and also the package itself.

@qiluo-msft qiluo-msft merged commit 4bda0a9 into sonic-net:master Oct 27, 2021
@qiluo-msft qiluo-msft deleted the qiluo/pipisntall branch October 27, 2021 04:12
judyjoseph pushed a commit that referenced this pull request Oct 28, 2021
…up.py install` (#8997)

Fix a recent build error introduced by a pre-release redis-py. This is a general issue because `python setup.py install` (ie `easy_instal`) does not ignore pre-release versions. The fix is suggested by pypa/setuptools#855 (comment)
qiluo-msft added a commit that referenced this pull request Oct 30, 2021
…up.py install` (#9111)

#### Why I did it
Backport #8997 to 202012 branch.
guxianghong pushed a commit to CentecNetworks/sonic-buildimage that referenced this pull request May 21, 2022
liuyuefengcn added a commit to liuyuefengcn/sonic-net-buildimage that referenced this pull request Nov 14, 2023
SHA-1: 798910a
* [build] Use pip to install setup.py dependency instead of `python setup.py install` (sonic-net#9111)
#### Why I did it
Backport sonic-net#8997 to 202012 branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants