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.

Sign up for GitHub

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

Jump to bottom

Introducing 'debugging' and 'profiling' options in sonic build-infra #1782

Merged
merged 3 commits into from
Jun 19, 2018
Merged

Introducing 'debugging' and 'profiling' options in sonic build-infra #1782

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d37d0da
Introducing 'debugging' and 'profiling' options in sonic build-infra
Jun 4, 2018
5173b2a
Addressing review comments.
Jun 18, 2018
28493ba
Addressing minor comment.
Jun 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions rules/config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ DEFAULT_PASSWORD = YourPaSsWoRd
# by default for TOR switch
# ENABLE_PFCWD_ON_START = y

# SONIC_CONFIG_DEBUG - install debug packages
# SONIC_INSTALL_DEBUG_TOOLS - installs debugging tools in baseline docker
# Uncomment next line to enable:
# SONIC_CONFIG_DEBUG = y
# SONIC_INSTALL_DEBUG_TOOLS = y

# SONIC_ROUTING_STACK - specify the routing-stack being elected to drive SONiC's control-plane.
# Quagga will be the default routing-stack for all the SONiC platforms. Other supported
Expand All @@ -56,5 +56,13 @@ SONIC_ROUTING_STACK = quagga
# Enable Origanization Extensions - Specific to the deployment scenarios of the Organization
ENABLE_ORGANIZATION_EXTENSIONS = y

# Debugging option allows sonic debian packages to get built including symbols
# information. Profiling option, disables compiler optimizations (-O0) as well
# as includes symbols information. Being 'profiling' option a superset of
Copy link
Contributor

Choose a reason for hiding this comment

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

Being 'profiling' option is a superset... [Missing "is"]

# 'debugging' one, user should only enable either one option or the other --
# if both options are enabled, the 'profiling' one will prevail.
#SONIC_DEBUGGING_ON = y
Copy link
Contributor

@jleveque jleveque Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I feel as though SONIC_DEBUGGING_ON is a bit confusing, as we also have the SONIC_CONFIG_DEBUG option at line 43, which installs helpful packages for debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about adding a better a description for SONIC_CONFIG_DEBUG option too? i can do something like: s/install debug packages/Install linux debugging tools/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we can also change SONIC_DEBUGGING_ON to something else. Let me know if have a better name for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change SONIC_CONFIG_DEBUG to something like SONIC_CONFIG_DEBUG_TOOLS, SONIC_CONFIG_INSTALL_DEBUG_TOOLS or SONIC_INSTALL_DEBUG_TOOLS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will go ahead with this one: SONIC_INSTALL_DEBUG_TOOLS

#SONIC_PROFILING_ON = y

# ENABLE_SYSTEM_TELEMETRY - build docker-sonic-telemetry for system telemetry support
# ENABLE_SYSTEM_TELEMETRY = y
2 changes: 1 addition & 1 deletion rules/docker-base.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ $(DOCKER_BASE)_PATH = $(DOCKERS_PATH)/docker-base
$(DOCKER_BASE)_DEPENDS += $(SUPERVISOR)
$(DOCKER_BASE)_DEPENDS += $(LIBWRAP)

ifeq ($(SONIC_CONFIG_DEBUG),y)
ifeq ($(SONIC_INSTALL_DEBUG_TOOLS),y)
GDB = gdb
VIM = vim
OPENSSH = openssh-client
Expand Down
18 changes: 14 additions & 4 deletions slave.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,14 @@ else
$(warning PASSWORD given on command line: could be visible to other users)
endif

ifeq ($(SONIC_DEBUGGING_ON),y)
DEB_BUILD_OPTIONS_GENERIC := "nostrip"
endif

ifeq ($(SONIC_PROFILING_ON),y)
DEB_BUILD_OPTIONS_GENERIC := "nostrip noopt"
endif

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to have both options enabled at the same time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would you want to do that? 'profiling' option is a superset of 'debugging' one, so i can't think about a use-case to enable both. Let me know if i didn't understand your question correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the concern is that the 'profiling' option is a superset of 'debugging, yet there is nothing preventing someone from enabling both. Maybe just enhance the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I'll clarify that in rules/config. Basically, if you enable both options, then only the 'superset' one (profiling) will be activated, as this one appears in slave.mk after 'debugging' option -- i placed them in this order on purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, just as an indication of how much larger sonic-broadcom.bin would get with 'debugging' option turned on: we are seeing an increment from the current ~420MB to ~495MB, which in our case definitely pays off to make our life easier at t-shooting time -- in fact we left 'debugging' option enabled by default.

ifeq ($(SONIC_BUILD_JOBS),)
override SONIC_BUILD_JOBS := $(SONIC_CONFIG_BUILD_JOBS)
endif
Expand All @@ -109,13 +117,15 @@ $(info "PASSWORD" : "$(PASSWORD)")
$(info "ENABLE_DHCP_GRAPH_SERVICE" : "$(ENABLE_DHCP_GRAPH_SERVICE)")
$(info "SHUTDOWN_BGP_ON_START" : "$(SHUTDOWN_BGP_ON_START)")
$(info "ENABLE_PFCWD_ON_START" : "$(ENABLE_PFCWD_ON_START)")
$(info "SONIC_CONFIG_DEBUG" : "$(SONIC_CONFIG_DEBUG)")
$(info "SONIC_INSTALL_DEBUG_TOOLS" : "$(SONIC_INSTALL_DEBUG_TOOLS)")
$(info "ROUTING_STACK" : "$(SONIC_ROUTING_STACK)")
$(info "ENABLE_SYNCD_RPC" : "$(ENABLE_SYNCD_RPC)")
$(info "ENABLE_ORGANIZATION_EXTENSIONS" : "$(ENABLE_ORGANIZATION_EXTENSIONS)")
$(info "HTTP_PROXY" : "$(HTTP_PROXY)")
$(info "HTTPS_PROXY" : "$(HTTPS_PROXY)")
$(info "ENABLE_SYSTEM_TELEMETRY" : "$(ENABLE_SYSTEM_TELEMETRY)")
$(info "SONIC_DEBUGGING_ON" : "$(SONIC_DEBUGGING_ON)")
$(info "SONIC_PROFILING_ON" : "$(SONIC_PROFILING_ON)")
$(info )

###############################################################################
Expand Down Expand Up @@ -201,7 +211,7 @@ $(addprefix $(DEBS_PATH)/, $(SONIC_MAKE_DEBS)) : $(DEBS_PATH)/% : .platform $$(a
# Apply series of patches if exist
if [ -f $($*_SRC_PATH).patch/series ]; then pushd $($*_SRC_PATH) && QUILT_PATCHES=../$(notdir $($*_SRC_PATH)).patch quilt push -a; popd; fi
# Build project and take package
make DEST=$(shell pwd)/$(DEBS_PATH) -C $($*_SRC_PATH) $(shell pwd)/$(DEBS_PATH)/$* $(LOG)
DEB_BUILD_OPTIONS="${DEB_BUILD_OPTIONS_GENERIC}" make DEST=$(shell pwd)/$(DEBS_PATH) -C $($*_SRC_PATH) $(shell pwd)/$(DEBS_PATH)/$* $(LOG)
# Clean up
if [ -f $($*_SRC_PATH).patch/series ]; then pushd $($*_SRC_PATH) && quilt pop -a -f; popd; fi
$(FOOTER)
Expand All @@ -224,8 +234,8 @@ $(addprefix $(DEBS_PATH)/, $(SONIC_DPKG_DEBS)) : $(DEBS_PATH)/% : .platform $$(a
pushd $($*_SRC_PATH) $(LOG)
[ ! -f ./autogen.sh ] || ./autogen.sh $(LOG)
$(if $($*_DPKG_TARGET),
DEB_BUILD_OPTIONS=$($*_DEB_BUILD_OPTIONS) dpkg-buildpackage -rfakeroot -b -us -uc -j$(SONIC_CONFIG_MAKE_JOBS) --as-root -T$($*_DPKG_TARGET) $(LOG),
DEB_BUILD_OPTIONS=$($*_DEB_BUILD_OPTIONS) dpkg-buildpackage -rfakeroot -b -us -uc -j$(SONIC_CONFIG_MAKE_JOBS) $(LOG)
DEB_BUILD_OPTIONS="${DEB_BUILD_OPTIONS_GENERIC} ${$*_DEB_BUILD_OPTIONS}" dpkg-buildpackage -rfakeroot -b -us -uc -j$(SONIC_CONFIG_MAKE_JOBS) --as-root -T$($*_DPKG_TARGET) $(LOG),
DEB_BUILD_OPTIONS="${DEB_BUILD_OPTIONS_GENERIC} ${$*_DEB_BUILD_OPTIONS}" dpkg-buildpackage -rfakeroot -b -us -uc -j$(SONIC_CONFIG_MAKE_JOBS) $(LOG)
)
popd $(LOG)
# Clean up
Expand Down