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

Fix INSTALL_DEBUG_TOOLS build config option #2564

Merged
merged 3 commits into from
Aug 15, 2019

Conversation

jipanyang
Copy link
Collaborator

Signed-off-by: Jipan Yang jipan.yang@alibaba-inc.com

- What I did
INSTALL_DEBUG_TOOLS, while not SONIC_INSTALL_DEBUG_TOOLS should be used in rules/config to make the option work.

- How I did it

- How to verify it

jipan@sonicvm1:~/upstream/sonic-buildimage$ make list
+++ --- Making list --- +++
BLDENV=stretch make -f Makefile.work stretch
make[1]: Entering directory '/home/jipan/upstream/sonic-buildimage'
SONiC Build System

Build Configuration
"CONFIGURED_PLATFORM"             : "broadcom"
"SONIC_CONFIG_PRINT_DEPENDENCIES" : ""
"SONIC_BUILD_JOBS"                : "1"
"SONIC_CONFIG_MAKE_JOBS"          : "8"
"USERNAME"                        : "admin"
"PASSWORD"                        : "YourPaSsWoRd"
"ENABLE_DHCP_GRAPH_SERVICE"       : ""
"SHUTDOWN_BGP_ON_START"           : ""
"ENABLE_PFCWD_ON_START"           : ""
"INSTALL_DEBUG_TOOLS"             : "y"
"ROUTING_STACK"                   : "quagga"
"ENABLE_SYNCD_RPC"                : ""
"ENABLE_ORGANIZATION_EXTENSIONS"  : "y"
"HTTP_PROXY"                      : ""
"HTTPS_PROXY"                     : ""
"ENABLE_SYSTEM_TELEMETRY"         : ""
"SONIC_DEBUGGING_ON"              : ""
"SONIC_PROFILING_ON"              : ""
"KERNEL_PROCURE_METHOD"           : "build"
"BUILD_TIMESTAMP"                 : "20190213.141507"
"BLDENV"                          : "stretch"

make: Nothing to be done for 'stretch'.
make[1]: Leaving directory '/home/jipan/upstream/sonic-buildimage'
make -f Makefile.work list
make[1]: Entering directory '/home/jipan/upstream/sonic-buildimage'
SONiC Build System

Build Configuration
"CONFIGURED_PLATFORM"             : "broadcom"
"SONIC_CONFIG_PRINT_DEPENDENCIES" : ""
"SONIC_BUILD_JOBS"                : "1"
"SONIC_CONFIG_MAKE_JOBS"          : "8"
"USERNAME"                        : "admin"
"PASSWORD"                        : "YourPaSsWoRd"
"ENABLE_DHCP_GRAPH_SERVICE"       : ""
"SHUTDOWN_BGP_ON_START"           : ""
"ENABLE_PFCWD_ON_START"           : ""
"INSTALL_DEBUG_TOOLS"             : "y"
"ROUTING_STACK"                   : "quagga"
"ENABLE_SYNCD_RPC"                : ""
"ENABLE_ORGANIZATION_EXTENSIONS"  : "y"
"HTTP_PROXY"                      : ""
"HTTPS_PROXY"                     : ""
"ENABLE_SYSTEM_TELEMETRY"         : ""
"SONIC_DEBUGGING_ON"              : ""
"SONIC_PROFILING_ON"              : ""
"KERNEL_PROCURE_METHOD"           : "build"
"BUILD_TIMESTAMP"                 : "20190213.141508"
"BLDENV"                          : ""

- Description for the changelog

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

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>
@jleveque
Copy link
Contributor

Along with this fix, can we also simplify the other makefiles? For example:

  • Can Makefile.work line 114 simply become INSTALL_DEBUG_TOOLS=$(INSTALL_DEBUG_TOOLS) \ and then also remove the following lines (88-90) from slave.mk?

    ifeq ($(SONIC_INSTALL_DEBUG_TOOLS),y)
    INSTALL_DEBUG_TOOLS = y
    endif
    

Or is there a reason we still need this?

@rodnymolina
Copy link
Contributor

@jleveque, @jipanyang if i remember correctly, at the time i added this logic i was trying to offer two approaches to enable this flag: 1) through environment-variable, 2) through rules/config variable. I'm perfectly fine if we want to eliminate option 1), and in that case i agree with you that lines 88-90 are not needed anymore.

@rodnymolina rodnymolina self-requested a review February 14, 2019 00:29
@jleveque
Copy link
Contributor

@rodnymolina: Was it the environment variable that required this setup? We also set a few other variables this way. I'm just curious whether this procedure is needed for the others, as well.

@rodnymolina
Copy link
Contributor

@jleveque Yep, this procedure is needed for the others i'm afraid. Generally speaking i think it's a good idea to reduce to the minimum the number of building-options that we expose through env-vars. There may be some good exceptions, but for the general case, we should default to rules/config as the go-to place.

@jipanyang
Copy link
Collaborator Author

@jleveque @rodnymolina I think we introduced the interim variable SONIC_xxxx to fix issue like #1615 .
Actually with this line of change: https://github.com/Azure/sonic-buildimage/blob/master/Makefile.work#L81 before SONIC_BUILD_INSTRUCTION, that has become unnecessary (?).

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>
@jipanyang
Copy link
Collaborator Author

jipan@sonicvm1:~/upstream/sonic-buildimage$ make configure PLATFORM=broadcom
+++ Making configure +++
make -f Makefile.work configure
make[1]: Entering directory '/home/jipan/upstream/sonic-buildimage'
SONiC Build System

Build Configuration
"CONFIGURED_PLATFORM"             : "broadcom"
"SONIC_CONFIG_PRINT_DEPENDENCIES" : ""
"SONIC_BUILD_JOBS"                : "1"
"SONIC_CONFIG_MAKE_JOBS"          : "8"
"USERNAME"                        : "admin"
"PASSWORD"                        : "YourPaSsWoRd"
"ENABLE_DHCP_GRAPH_SERVICE"       : ""
"SHUTDOWN_BGP_ON_START"           : ""
"ENABLE_PFCWD_ON_START"           : "y"
"INSTALL_DEBUG_TOOLS"             : "y"
"ROUTING_STACK"                   : "quagga"
"ENABLE_SYNCD_RPC"                : ""
"ENABLE_ORGANIZATION_EXTENSIONS"  : "y"
"HTTP_PROXY"                      : ""
"HTTPS_PROXY"                     : ""
"ENABLE_SYSTEM_TELEMETRY"         : "y"
"SONIC_DEBUGGING_ON"              : ""
"SONIC_PROFILING_ON"              : ""
"KERNEL_PROCURE_METHOD"           : "build"
"BUILD_TIMESTAMP"                 : "20190213.182842"
"BLDENV"                          : ""

make[1]: Leaving directory '/home/jipan/upstream/sonic-buildimage

Makefile.work Outdated
SONIC_INSTALL_DEBUG_TOOLS=$(INSTALL_DEBUG_TOOLS) \
ENABLE_PFCWD_ON_START=$(ENABLE_PFCWD_ON_START) \
ENABLE_SYNCD_RPC=$(ENABLE_SYNCD_RPC) \
INSTALL_DEBUG_TOOLS=$(INSTALL_DEBUG_TOOLS) \
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not correct. if ENABLE_PFCWD_ON_START is not defined, then it will override the default setting in rules/config.

rules/config Show resolved Hide resolved
slave.mk Outdated Show resolved Hide resolved
Copy link
Collaborator

@lguohan lguohan 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.

@lguohan
Copy link
Collaborator

lguohan commented Feb 14, 2019

@jipanyang , it looks like you are correct, with the include rules/config in Makefile.work. However, we are not passing variable into slave.mk. we should include rules/config only once, now, it looks like we include twice, one in Makefile.work and one in slave.mk. I am not sure it is correct to have rules/config include in both place. It was an oversight.

@jipanyang
Copy link
Collaborator Author

@lguohan Limited the scope of this PR to "INSTALL_DEBUG_TOOLS" only.

@lguohan lguohan merged commit 0ec5de4 into sonic-net:master Aug 15, 2019
wangshengjun pushed a commit to wangshengjun/sonic-buildimage that referenced this pull request Nov 16, 2020
* Fix INSTALL_DEBUG_TOOLS build config option

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>
dgsudharsan added a commit to dgsudharsan/sonic-buildimage that referenced this pull request Dec 20, 2022
Update sonic-utilities submodule pointer to include the following:
* 67cbb15 [202205]Fixes sonic-net#12170: Delete subinterface and recreate the subinterface in default-vrf ([sonic-net#2564](sonic-net/sonic-utilities#2564))
* 93172c4 [202205] [generate_dump] Optimize the execution time of the 'show techsupport' script to 5-10% by reducing calls to the 'tar append' operation ([sonic-net#2562](sonic-net/sonic-utilities#2562))

Signed-off-by: dgsudharsan <sudharsand@nvidia.com>
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Dec 26, 2022
Update sonic-utilities submodule pointer to include the following:
* f7988b0 [202205] [timer.unit.j2] use wanted-by in timer unit ([sonic-net#2561](sonic-net/sonic-utilities#2561))
* f45dcfb [generate_dump] Optimize the execution time of 'show techsupport' CLI by paraller function execution ([sonic-net#2565](sonic-net/sonic-utilities#2565))
* 67cbb15 [202205]Fixes sonic-net#12170: Delete subinterface and recreate the subinterface in default-vrf ([sonic-net#2564](sonic-net/sonic-utilities#2564))
* 93172c4 [202205] [generate_dump] Optimize the execution time of the 'show techsupport' script to 5-10% by reducing calls to the 'tar append' operation ([sonic-net#2562](sonic-net/sonic-utilities#2562))

Signed-off-by: dprital <drorp@nvidia.com>
liat-grozovik pushed a commit that referenced this pull request Jan 1, 2023
Update sonic-utilities submodule pointer to include the following:

3bc2bc6 [Mellanox][202205] Change severity to NOTICE in Mellanox buffer migrator when unable to fetch DEVICE_METADATA due to empty CONFIG_DB during initialization (#2570)
e1c8243 [202205][generate_dump] Fix for a deletion flow for all secret files in the techsupport dump (#2572)
9f2984a [202205] Fix issue: unconfigured PGs are displayed in watermarkstat (#2568)
f7988b0 [202205] [timer.unit.j2] use wanted-by in timer unit (#2561)
f45dcfb [generate_dump] Optimize the execution time of 'show techsupport' CLI by paraller function execution (#2565)
67cbb15 [202205]Fixes 12170: Delete subinterface and recreate the subinterface in default-vrf (#2564)
93172c4 [202205] [generate_dump] Optimize the execution time of the 'show techsupport' script to 5-10% by reducing calls to the 'tar append' operation (#2562)

Signed-off-by: dprital <drorp@nvidia.com>
mssonicbld added a commit that referenced this pull request Jul 24, 2023
…lly (#15943)

#### Why I did it
src/sonic-swss
```
* 6ec611dc - (HEAD -> 202211, origin/202211) [202211][ppi]: Implement port bulk comparison logic (#2564) (#2821) (19 hours ago) [Nazarii Hnydyn]
```
#### How I did it
#### How to verify it
#### Description for the changelog
StormLiangMS pushed a commit that referenced this pull request Aug 30, 2023
DEPENDS:

[202211][ppi]: Implement port bulk comparison logic (#2564)  sonic-swss#2821
HLD: sonic-net/SONiC#1084

Why I did it
Enabled port late create on SN5600 switch boots up with no ports
Work item tracking
N/A
How I did it
Updated SAI xml config file
How to verify it
Run sonic-mgmt tests fastboot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants