-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Click Upgrade to 7.0.0 #3907
Click Upgrade to 7.0.0 #3907
Conversation
Signed-off-by: Sangita Maity <sangitamaity0211@gmail.com>
@samaity: With this solution, do we wind up with two versions of Click installed on the system (a older version via apt-get, the this one via pip)? The issue is that sonic-utilities is build as a .deb package, so it attempts to install its dependencies as .deb files. This was the reason why I opted to build the .deb file from source. See #1824 |
retest vsimage please |
Retest vsimage please |
@jleveque, As far as I see using
|
Yes. Run |
the easiest change is to something similar to this one. |
Yes @jleveque, you are right. I have another version seating there. |
yes, I am testing it out. As far as I remember, I tried to do something similar to this, but I was experiencing some problems build the deb package. will update once I am done with this. Thanks, @lguohan !!! |
Do we have updates on this pr? |
Hi @jleveque @lguohan @stevenlu99,
My suggestion would be to remove the install requirement of click from sonic-utilities setup.py file. As we are already installing click via pip and once, we move the dependency requirement, sonic-utilities is built as a .deb package, it will not attempt to install click once again. So, we will not have two versions of Click installed on the system. Let me know how it sounds. |
@samaity: This brings us back to a conundrum we have faced a few times in the past. Ultimately, we would like to properly configure dependencies for all of our packages. However, if we build a Python package as a .deb package, it looks for its dependencies as other installed .deb packages. The opposite goes for building as a .whl -- it will look for its dependencies as installed .whls. I tried building sonic-utilities as a .whl in the past -- this way we could install all Python packages as .whls. However, .whl packages are not allowed to install files outside of the /usr/local/lib/python2.7/dist-packages/ directory, which is something we do with this package. So, we can do as you propose (install Click as a .whl and remove it from the sonic-utilities dependencies), but we lose the intrinsic check for dependencies (I must admit, we have already done this for a couple other dependencies, which I have tried to make clear in the setup.py file). It's not ideal, but we could make it work. A bigger, separate concern for me is that currently we are building Click 6.7-4 from source because I added some patches to fix some bugs. The Click folks have since accepted and merged my patches in their upstream codebase. First, we need to confirm that my fixes made it into Click 7.0. If so, that's great. If not, we will need to continue building from source; in which case, we will have the same dependency issues you encountered until we move to Buster. |
Hey @jleveque , I checked the Click 7.0 code. I see the fixes incorporated there so we won't get any bug regarding CLI autocomplete/suggest. So, we are good there. I have removed |
@samaity , can you change your pr to build the deb package? |
obseleted by 4e07c78 |
Signed-off-by: Sangita Maity sangitamaity0211@gmail.com
- What I did
Upgraded click to 7.0.0 via pip. This is needed to support autocompletion part in
breakout
cli command to support dynamic option for mode choice.- How to verify it
Built python-sonic-utilities_1.2-1_all.deb package successfully.
checked click version in newly built
sonic-slave
docker.