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 python-click Debian package from version 6.7-4 source to fix CLI autocomplete/suggest #1824

Merged
merged 2 commits into from
Jun 29, 2018
Merged

Conversation

jleveque
Copy link
Contributor

@jleveque jleveque commented Jun 28, 2018

- What I did

  • Download python-click source from Debian repos and build .deb package using v6.7-4 tag.
  • There exists a bug in versions of Click < 6.7 in which bash completion (tab-to-autocomplete/tab-to-suggest) only works for the first two levels of subcommands. The bug was fixed in v6.7, however, the most recent package available via apt for Debian Jessie and Stretch is currently v6.6-1. so to get the fix, we must build our own package.

- How to verify it

  • Try pressing Tab once to autocomplete commands at >= 3 levels of subgroups/subcommands

  • Example:

    admin@sonic:~$ show interfaces transceiver ee
    

    Press Tab once...

    admin@sonic:~$ show interfaces transceiver eeprom
    
  • Try pressing Tab twice to suggest subcommands at >= 3 levels of subgroups/subcommands

    admin@sonic:~$ show interfaces transceiver
    

    Press Tab twice...

    admin@sonic:~$ show interfaces transceiver 
    eeprom    lpmode    presence  
    
  • Try pressing Tab twice to suggest options at >= 3 levels of subgroups/subcommands

    admin@sonic:~$ show interfaces transceiver eeprom -
    

    Press Tab twice...

    admin@sonic:~$ show interfaces transceiver eeprom -
    --dom      --verbose  -d 
    

- Unrelated

  • Remove Python3 dependency for sonic-utilities, as we are no longer building a Python3 version of that package.

# an older version as part of its dependencies
sudo dpkg --root=$FILESYSTEM_ROOT -i target/debs/python-click*_all.deb || \
sudo LANG=C DEBIAN_FRONTEND=noninteractive chroot $FILESYSTEM_ROOT apt-get -y install -f

Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 28, 2018

Choose a reason for hiding this comment

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

Prefer

pip install click>=6.7

if possible

Copy link
Contributor Author

@jleveque jleveque Jun 28, 2018

Choose a reason for hiding this comment

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

It's possible, but because sonic-utilities is built as a .deb package, it attempts to install its dependencies as .deb packages, also. We've encountered this in the past. If we install the newer Click via pip, we will wind up with two versions of Click installed on the system. The default Python paths should look in the pip installation location (/usr/local/lib/python2.7/dist-packages) before the apt installation location (/usr/lib/python2.7/dist-packages), so in theory it would work, but it's messy and provides the potential for the wrong version to be used if the paths are not as expected. This solution ensures only the 6.7 version of Click is installed on the system, so we have no need to worry about unexpected behavior.

python3-docutils \
python3-requests \
python3-pytest \
python3-colorama
Copy link
Collaborator

Choose a reason for hiding this comment

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

some are already installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't hurt to specify them more than once, and this way it's explicit which packages are required by other packages. If in the future we have no need for a package and someone removes its dependencies here, shared dependencies will still be listed under the other packages which require them.

@@ -213,7 +211,17 @@ RUN apt-get update && apt-get install -y \
linuxdoc-tools \
lynx \
texlive-latex-extra \
texlive-latex-recommended
texlive-latex-recommended\
Copy link
Collaborator

Choose a reason for hiding this comment

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

add one blank

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. Thanks, I somehow missed that.

Copy link
Collaborator

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

As comments.

@jleveque jleveque merged commit f04f070 into sonic-net:master Jun 29, 2018
@jleveque jleveque deleted the upgrade_click branch June 29, 2018 16:59
@jleveque jleveque mentioned this pull request Dec 16, 2019
stephenxs added a commit to stephenxs/sonic-buildimage that referenced this pull request Jul 29, 2021
237b89c [Dynamic Buffer Calc] Bug fix: Don't create lossless buffer profile for active ports without speed configured (sonic-net#1822)
ec104c1 [gearbox] Set context for phys based on configs (sonic-net#1826)
7f80f06 Td2: Reclaim buffer from unused ports (sonic-net#1830)
0fe2dfe [VS] Fix for VS test failures (sonic-net#1836)
c81e319 refactor(fdbsyncd): Convert files with dos2unix (sonic-net#1828)
c805021 [configure.ac] Add the option of passing libnl path to configure script (sonic-net#1824)
ed6786d 9f0bb8d [swss]: Allow portsyncd to run on system without ports (sonic-net#1808)
5d97b05 Update MACsec SA PN counter to support SAI API 1.8 (sonic-net#1818)
64e33b3 Ignore ALREADY_EXIST error in FDB creation (sonic-net#1815)

Signed-off-by: Stephen Sun <stephens@nvidia.com>
theasianpianist pushed a commit to theasianpianist/sonic-buildimage that referenced this pull request Feb 5, 2022
…pt (sonic-net#1824)

**What I did**
Add the option of passing custmol built libnl path to configure script

**Why I did it**
MPLS feature in sonic-buildimage requires a libnl patch to be applied before building libnl. Since this build is not installed in usual locations (/usr/lib/..) LGTM analysis fails. This change gives the option of passing libnl library location to 'configure' script and generate libraries to be linked.

In case the options are not passed, the configure script defaults to earlier behavior where it checks for LIBNL in usual locations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tab-based autocompletion only works for first two subcommands/subgroups
3 participants