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

[sonic-host-service] Move to sonic-host-services package #6273

Merged
merged 11 commits into from
Feb 9, 2021

Conversation

ArunSaravananBalachandran
Copy link
Contributor

- Why I did it

To move ‘sonic-host-service’ which is currently built as a separate package to ‘sonic-host-services' package.

- How I did it

  • Moved 'sonic-host-server' to 'src/sonic-host-services' and included it as part of the python3 wheel.
  • Other files were moved to 'src/sonic-host-services-data' and included as part of the deb package.
  • Changed build option ‘INCLUDE_HOST_SERVICE’ to ‘ENABLE_HOST_SERVICE_ON_START’ for enabling sonic-hostservice at boot-up by default.

- How to verify it

Ensure that 'sonic-host-server' is installed and runs based on the build option.

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

  • 201811
  • 201911
  • 202006

- Description for the changelog

[sonic-host-service] Move to sonic-host-services package

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

@@ -7,5 +7,5 @@ Build-Depends: debhelper (>=11)

Package: sonic-host-services-data
Architecture: all
Depends: ${misc:Depends}
Depends: python3-systemd, python3-dbus, python3-gi, ${misc:Depends}
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than specifying these Python dependencies as Debian packages here, I prefer if you add them to the install_requires= list of sonic-host-services/setup.py. This way they will automatically get installed by pip with the sonic-host-services package, which is the package that really requires them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Above python modules are dependent on other Debian packages that are not available by default and they are required to be installed prior to installing the former using pip.
[ Dependencies: systemd, PyGObject ]

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, these types of Debian package dependencies are a pain. In other instances, we install the Debian package dependencies explicitly before installing the Python wheel and then let pip install the Python dependencies. Let me think about this a bit more.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should now be able to move these Python dependencies to the setup.py file in the install_requires list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Above python dependencies require other non-default debian packages for build and including the former in install_requires causes the sonic-host-services build to fail.

--- a/src/sonic-host-services/setup.py
+++ b/src/sonic-host-services/setup.py
@@ -25,6 +25,7 @@ setup(
         'Jinja2>=2.10',
         'sonic-py-common',
         'swsssdk>=2.0.1',
+        'PyGObject'
     ],
     setup_requires = [
         'pytest-runner',
[ FAIL LOG START ] [ target/python-wheels/sonic_host_services-1.0-py3-none-any.whl ]
[ REASON ] :      target/python-wheels/sonic_host_services-1.0-py3-none-any.whl does not exist   NON-EXISTENT PREREQUISITES: target/python-wheels/sonic_py_common-1.0-py3-none-any.whl-install target/python
-wheels/swsssdk-2.0.1-py3-none-any.whl-install
[ FLAGS  FILE    ] : []
[ FLAGS  DEPENDS ] : []
[ FLAGS  DIFF    ] : []
/sonic/src/sonic-host-services /sonic
running pytest
Searching for pycairo>=1.11.1
Reading https://pypi.org/simple/pycairo/
Downloading https://files.pythonhosted.org/packages/9d/6e/499d6a6db416eb3cdf0e57762a269908e4ab6638a75a90972afc34885b91/pycairo-1.20.0.tar.gz#sha256=5695a10cb7f9ae0d01f665b56602a845b0a8cb17e2123bfece10c2e5
8552468c
Best match: pycairo 1.20.0
Processing pycairo-1.20.0.tar.gz
Writing /tmp/easy_install-ia2gxuvw/pycairo-1.20.0/setup.cfg
Running pycairo-1.20.0/setup.py -q bdist_egg --dist-dir /tmp/easy_install-ia2gxuvw/pycairo-1.20.0/egg-dist-tmp-cfcfxbbc
no previously-included directories found matching 'docs/_build'
warning: no files found matching 'README' under directory 'tests'
warning: no files found matching 'README' under directory 'examples'
Package cairo was not found in the pkg-config search path.
Perhaps you should add the directory containing `cairo.pc'
to the PKG_CONFIG_PATH environment variable
No package 'cairo' found
error: Setup script exited with Command '['pkg-config', '--print-errors', '--exists', 'cairo >= 1.15.10']' returned non-zero exit status 1.
[  FAIL LOG END  ] [ target/python-wheels/sonic_host_services-1.0-py3-none-any.whl ]

Copy link
Contributor

Choose a reason for hiding this comment

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

@ArunSaravananBalachandran: Will it work if you install the Debian dependencies before installing the sonic-host-services package in sonic_debian_extension.j2, like the following?

# Install Debian dependencies of SONiC Utilities Python package
sudo LANG=C DEBIAN_FRONTEND=noninteractive chroot $FILESYSTEM_ROOT apt-get install libcairo2-dev

# Install SONiC host services package
SONIC_HOST_SERVICES_PY3_WHEEL_NAME=$(basename {{sonic_host_services_py3_wheel_path}})
sudo cp {{sonic_host_services_py3_wheel_path}} $FILESYSTEM_ROOT/$SONIC_HOST_SERVICES_PY3_WHEEL_NAME
sudo https_proxy=$https_proxy LANG=C chroot $FILESYSTEM_ROOT pip3 install $SONIC_HOST_SERVICES_PY3_WHEEL_NAME
sudo rm -rf $FILESYSTEM_ROOT/$SONIC_HOST_SERVICES_PY3_WHEEL_NAME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jleveque , the failure is observed during the unit test, even before the whl is generated.
https://github.com/Azure/sonic-buildimage/blob/1dcab4d1e3a04f07d72b4cc1d64da081ea5aa329/slave.mk#L607-L608

root@cda937ba715b:/sonic/src/sonic-host-services# python3 setup.py test
running pytest
Searching for pycairo>=1.11.1
Reading https://pypi.org/simple/pycairo/
Downloading https://files.pythonhosted.org/packages/9d/6e/499d6a6db416eb3cdf0e57762a269908e4ab6638a75a90972afc34885b91/pycairo-1.20.0.tar.gz#sha256=5695a10cb7f9ae0d01f665b56602a845b0a8cb17e2123bfece10c2e58552468c
Best match: pycairo 1.20.0
Processing pycairo-1.20.0.tar.gz
Writing /tmp/easy_install-5ydjvscm/pycairo-1.20.0/setup.cfg
Running pycairo-1.20.0/setup.py -q bdist_egg --dist-dir /tmp/easy_install-5ydjvscm/pycairo-1.20.0/egg-dist-tmp-lphftox9
no previously-included directories found matching 'docs/_build'
warning: no files found matching 'README' under directory 'tests'
warning: no files found matching 'README' under directory 'examples'
Package cairo was not found in the pkg-config search path.
Perhaps you should add the directory containing `cairo.pc'
to the PKG_CONFIG_PATH environment variable
No package 'cairo' found
error: Setup script exited with Command '['pkg-config', '--print-errors', '--exists', 'cairo >= 1.15.10']' returned non-zero exit status 1.
root@cda937ba715b:/sonic/src/sonic-host-services#

Copy link
Contributor

Choose a reason for hiding this comment

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

@ArunSaravananBalachandran: Then you probably also need to install the Debian packages required for testing/building in the slave container: https://github.com/Azure/sonic-buildimage/blob/master/sonic-slave-buster/Dockerfile.j2#L48-L340.

@jleveque
Copy link
Contributor

The file host_modules/host_service.py is being installed as part of the sonic-host-services-data package. Why not install host_modules as a package in sonic-host-services?

@joyas-joseph
Copy link
Contributor

The file host_modules/host_service.py is being installed as part of the sonic-host-services-data package. Why not install host_modules as a package in sonic-host-services?

Regarding the packages sonic-host-services and sonic-host-services-data packages:

  • There is a dependency between the 2 packages. The service files in data package expects the binary files to be installed in a certain location.
  • 'Wheel' packages are better suited for python modules. A regular Debian package fits better for sonic-host-services

Given these, I would prefer to have a single package that provides the binaries, service files and other meta/config files.

  • It will help avoid the dependency
  • It will help avoid the confusion like the one there is now - what exactly goes in sonic-host-services vs what goes into sonic-host-services-package?

@jleveque Is there any particular reason we went with 2 packages? Can we go ahead with a single package approach?

@jleveque
Copy link
Contributor

jleveque commented Jan 8, 2021

@jleveque Is there any particular reason we went with 2 packages? Can we go ahead with a single package approach?

We want to install all Python packages as wheel packages using pip so that pip will implicitly install all specified dependencies. Unfortunately, wheel packages can only install data files in a path relative to its install location; you cannot provide an absolute path (see https://packaging.python.org/guides/distributing-packages-using-setuptools/#data-files). So in order to install data files to locations outside of /usr/lib/python3.7/dist-packages, we have moved data files into a separate Debian package.

@ArunSaravananBalachandran
Copy link
Contributor Author

ArunSaravananBalachandran commented Jan 11, 2021

@jleveque, have pushed a commit to include 'host_modules' as a part of 'sonic-host-services' package.

src/sonic-host-services/setup.py Outdated Show resolved Hide resolved
Makefile.work Outdated Show resolved Hide resolved
files/build_templates/sonic_debian_extension.j2 Outdated Show resolved Hide resolved
    - Alphabetically arrange python dependent packages
    - Change 'SONIC_ENABLE_HOST_SERVICE_ON_START' to 'ENABLE_HOST_SERVICE_ON_START'
@ArunSaravananBalachandran
Copy link
Contributor Author

@jleveque, have pushed a commit to fix 'vs image' build failure due to sonic-host-services's dependencies.

@@ -246,6 +246,16 @@ sudo dpkg --root=$FILESYSTEM_ROOT -i $debs_path/sonic-utilities-data_*.deb || \
# in bash.bashrc, so we copy a version of the file with it enabled here.
sudo cp -f $IMAGE_CONFIGS/bash/bash.bashrc $FILESYSTEM_ROOT/etc/

# Remove 'PyGObject' package installed via apt
sudo LANG=C DEBIAN_FRONTEND=noninteractive chroot $FILESYSTEM_ROOT apt-get -y remove python3-gi
Copy link
Collaborator

@qiluo-msft qiluo-msft Feb 2, 2021

Choose a reason for hiding this comment

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

remove [](start = 78, length = 6)

replace remove with purge ? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed 'remove' to 'purge' for python3-gi

@@ -246,6 +246,16 @@ sudo dpkg --root=$FILESYSTEM_ROOT -i $debs_path/sonic-utilities-data_*.deb || \
# in bash.bashrc, so we copy a version of the file with it enabled here.
sudo cp -f $IMAGE_CONFIGS/bash/bash.bashrc $FILESYSTEM_ROOT/etc/

# Remove 'PyGObject' package installed via apt
sudo LANG=C DEBIAN_FRONTEND=noninteractive chroot $FILESYSTEM_ROOT apt-get -y remove python3-gi
Copy link
Collaborator

@qiluo-msft qiluo-msft Feb 2, 2021

Choose a reason for hiding this comment

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

python3-gi [](start = 85, length = 10)

Do you know which step install python3-gi ? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

python3-gi is installed as part of software-properties-common installation in build_debian.sh
https://github.com/Azure/sonic-buildimage/blob/eeb955489f7818ea2c21b86cbece3db2c76f5df5/build_debian.sh#L196-L200

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then I am confused, if installed in sonic-buildimage/build_debian.sh, we need to clean up in that file.
You mentioned "Remove 'PyGObject' package installed via apt", but I did not see the relationship.


In reply to: 568892398 [](ancestors = 568892398)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PyGObject python package is part of python3-gi and its installation via apt leads to ' PyGObject' not being installed via pip, while installing ‘sonic-host-services’. But ‘python3-gi’ is removed while ‘apt-get autoremove’ is executed during the final stages of the build and thus apt version of ‘PyGObject’ also gets uninstalled.

To avoid the above behavior, ‘python3-gi’ is removed before installing ‘sonic-host-services’ to install ‘PyGObject’ via pip itself.
It was placed here, since its removal was only required for installing 'sonic-host-services'.
Let me know if 'python3-gi' removal has to be placed in build_debian.sh, before invoking sonic_debian_extension.sh.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You reply shows great insight, really appreciate!
My understanding of build_debian.sh software-properties-common installation is required to add an apt-key and then later install docker engine. If you want the package removed, you may remove it in build_debian.sh immediately after the usage.

Since the package impact to python applications is unexpected, please also add a big comment block there with your insight here.


In reply to: 569194019 [](ancestors = 569194019,568892398)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As suggested, have moved the uninstallation of 'python3-gi' to build_debian.sh.

jleveque
jleveque previously approved these changes Feb 5, 2021
@jleveque jleveque requested a review from qiluo-msft February 5, 2021 21:18
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 comment

@jleveque jleveque merged commit 3015de1 into sonic-net:master Feb 9, 2021
@ArunSaravananBalachandran ArunSaravananBalachandran deleted the host_service branch August 24, 2021 05:56
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.

4 participants