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

Introduce gcovpreload repo to support code coverage report generation for swss and sairedis #11323

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

pettershao-ragilenetworks
Copy link
Contributor

Why I did it

This new component will be compiled as a dynamic library, then it can be used as a dependency for those components being required to generate code coverage report. This repo is migrated from sonic-sairedis repo and treated as a new repo in sonic-buildimage. The reason for that please check sonic-net/sonic-sairedis#972

How I did it

make gcovpreload as a new repo in sonic-buildimage and compile it to a dynamic library and a regular deb

How to verify it

After #972 merged, check whether sairedis can normally generate coverage report

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205

Description for the changelog

Introduce gcovpreload repo to support code coverage report generation for swss and sairedis

Link to config_db schema for YANG module changes

NA

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

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 5, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@qiluo-msft
Copy link
Collaborator

@xumia Could you review?

@pettershao-ragilenetworks
Copy link
Contributor Author

@qiluo-msft @lguohan @xumia Can you help check this pipeline compling issues? I didn't modify any files during compiling process, don't know why it is failing.

Add LIBGCOV_PRELOAD dependents in DOCKER_SONIC_VS_DEPENDS
@@ -58,3 +58,4 @@ if [ -f "$USE_PCI_ID_IN_CHASSIS_STATE_DB" ]; then
fi

exec /usr/local/bin/supervisord
echo /usr/lib/libgcovpreload.so > /etc/ld.so.preload
Copy link
Collaborator

@xumia xumia Sep 7, 2022

Choose a reason for hiding this comment

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

How about adding in the debian postinst step?
The package should be only for code coverage, not necessary to install it in product, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about adding in the debian postinst step? ------updated
The package should be only for code coverage, not necessary to install it in product, right? ----yes, this package is only for code coverage.

.gitignore Outdated
Comment on lines 47 to 52
# Subdirectories in src
src/preload/gcovpreload/debian/*
!src/preload/gcovpreload/debian/changelog
!src/preload/gcovpreload/debian/control
!src/preload/gcovpreload/debian/rules

Copy link
Collaborator

@xumia xumia Sep 7, 2022

Choose a reason for hiding this comment

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

How about adding in src/preload/.gitignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

change .gitignore from /.gitignore to /src/preload/..gitignore
change /.gitignore to /src/preload/.gitignore
Add libgcovpreload.postinst
Update .gitignore
use postinst to replace "echo /usr/lib/libgcovpreload.so > /etc/ld.so.preload"
use postinst to replace "echo /usr/lib/libgcovpreload.so > /etc/ld.so.preload"
use postinst to replace "echo /usr/lib/libgcovpreload.so > /etc/ld.so.preload"
use postinst to replace "echo /usr/lib/libgcovpreload.so > /etc/ld.so.preload"
To trigger rebuild&test
@pettershao-ragilenetworks pettershao-ragilenetworks requested review from xumia and removed request for lguohan and qiluo-msft September 19, 2022 09:33
@@ -13,7 +13,7 @@ endif
$(DOCKER_ORCHAGENT)_DBG_DEPENDS = $($(DOCKER_SWSS_LAYER_BULLSEYE)_DBG_DEPENDS)
$(DOCKER_ORCHAGENT)_DBG_DEPENDS += $(SWSS_DBG) \
$(LIBSWSSCOMMON_DBG) \
$(LIBSAIREDIS_DBG)
$(LIBSAIREDIS_DBG) \
Copy link
Collaborator

@xumia xumia Sep 21, 2022

Choose a reason for hiding this comment

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

Looks like the change is not necessary, may need to remove the last character "\", or miss a dependent target name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Have the same concern.

@@ -8,40 +8,43 @@ $(LIBSAIREDIS)_DPKG_TARGET = binary-sairedis
$(LIBSAIREDIS)_SRC_PATH = $(SRC_PATH)/sonic-sairedis
$(LIBSAIREDIS)_VERSION = $(LIBSAIREDIS_VERSION)
$(LIBSAIREDIS)_NAME = $(LIBSAIREDIS_NAME)
$(LIBSAIREDIS)_DEPENDS += $(LIBSWSSCOMMON_DEV)
$(LIBSAIREDIS)_DEPENDS += $(LIBSWSSCOMMON_DEV) $(LIBGCOV_PRELOAD)
Copy link
Collaborator

@xumia xumia Sep 21, 2022

Choose a reason for hiding this comment

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

Not sure my understanding is correct. Is it for code coverage? Is it possible to install it on demand, we can enable it in PR build, for official build to install in devices, looks like it is not necessary. We may need to clean some tools for build/test only in the image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, I have raised a sairedis PR to support GCOV, but need preload lib(the reason why can't build with sairedis repo together has mentioned in PR: sonic-net/sonic-sairedis#972). As I know, pipeline framework will download vs docker and install sairedis deb into it, and then do testing. So i need to make sure that the downloaded docker encompass preload lib.
Is it possible to install it on demand -> I think a MACRO "ENABLE_GCOV" defined in rules/config will be fine. I will add it soon, default value set to 'y'.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We do not want LIBGCOV_PRELOAD in the sonic image. You only need it in test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there other ways to solve this problem? to make sure that vs docker downloaded by pipeline framework when testing encompass gcov preload lib and this lib will not in sonic image ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not want LIBGCOV_PRELOAD in the sonic image. You only need it in test.

Do you have any idea about how to solve this problem?

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.

3 participants