RPC builds are broken

[jon-nokia]

Description

sonic-net/sonic-sairedis#1409 contained in latest HEAD is causing RPC build failure.

make ENABLE_SYNCD_RPC=y target/docker-syncd-brcm-dnx-rpc.gz

mkdir -p ./src/obj
g++ -I/usr/include/sai -I. -I../../experimental -std=c++11 -DFORCE_BOOST_SMART_PTR -c src/switch_sai_rpc_server.cpp -o src/obj/switch_sai_rpc_server.o -I/usr/include/sai -I. -I../../experimental -std=c++11 -DFORCE_BOOST_SMART_PTR -I./src/gen-cpp
In file included from ../../experimental/saiexperimentaldashvip.h:30,
from /usr/include/sai/saiobject.h:40,
from /usr/include/sai/sai.h:50,
from src/switch_sai_rpc_server.cpp:48:
../../experimental/saitypesextensions.h:37:46: error: 'SAI_OBJECT_TYPE_EXTENSIONS_RANGE_BASE' was not declared in this scope
37 | SAI_OBJECT_TYPE_EXTENSIONS_RANGE_START = SAI_OBJECT_TYPE_EXTENSIONS_RANGE_BASE,

Steps to reproduce the issue:

Describe the results you received:

Describe the results you expected:

Output of show version:

(paste your output here)

Output of show techsupport:

(paste your output here or download and attach the file here )

Additional information you deem important (e.g. issue happens only occasionally):

[saksarav-nokia]

@kcudnik for viz. I think your PR opencomputeproject/SAI#2028 is causing this.
We are trying to build RPC image for broadcom dnx platform with SAI 11.2.7.1

[bingwang-ms]

Issue is seen in master branch when building syncd with rpc enabled.
@kcudnik @liushilongbuaa Can you check if buildteam can help address this issue?

[liushilongbuaa]

@theasianpianist , please also help to check.

[arlakshm]

@saksarav-nokia, @jon-nokia Is this issue seen in 202405?

[jon-nokia]

@saksarav-nokia, @jon-nokia Is this issue seen in 202405?

No. I believe 202405 src/sonic-sairedis is still at d3fbec0, so has not had update since 5/27. src/sonic-sairedis commit, 433d039 brings in new SAI submodule which causes break.

[kcudnik]

not sure how this is cauing RPC error, seems like your headers are not updated, in my change you pointed https://github.com/opencomputeproject/SAI/pull/2028/files#diff-444febffce442e4277a1ced22d1ab94df045dfaa10a1b671ee35a7e5c2d9f683R305 this line implements SAI_OBJECT_TYPE_EXTENSIONS_RANGE_BASE, SAI headers build is passing on girhub and locally, also this is passing on sairedis repo where build is also passing, not sure how your build is including headers that are not including correct saitypes.h

[kcudnik]

can i reporoduce this locally without building entire image ?

[saksarav-nokia]

@kcudnik , i believe the libsai package installs the sai header files in /usr/include/sai and sai-thrift includes these header files for build. Since the vendor SAI header files don't have your changes, the build is failing.

[kcudnik]

so seems like vendor headers needs to be updated, or we need to update rpc build to use local headers, since originally rpc build code is not my code, if i remember correctly they copy /usr/include/sai headers to build rpc instead of local SAI repo headers, thats why it could fail

any suggestions which way we should go ?

[kcudnik]

problem is here: https://github.com/opencomputeproject/SAI/blob/master/test/saithrift/Makefile#L6 sai headers are taken form /usr/include/sai which are vendor headers, and experimental headers are taken from -I../../experimental as local headers to compile

but im not sure changing

SAI_PREFIX = /usr
SAI_HEADER_DIR ?= $(SAI_PREFIX)/include/sai
SAI_HEADERS = $(SAI_HEADER_DIR)/sai*.h
CFLAGS = -I$(SAI_HEADER_DIR) -I. -I../../experimental -std=c++11

will cause some other issues, since SAI_HEADER_DIR is with "?" which means i cloud be override externally before calling makefile, im not sure if DASH is utilizing this feature.

not sure what would be easy fix here, where should we patch
+@lguohan

this was added here: opencomputeproject/SAI@410a5bd for that specific reason

[saksarav-nokia]

@kcudnik , I am able to build RPC image for BCM DNX if i change the sai-thrift Makefile to use the experimental header files from /usr/include/sai/experimental. Not sure if all SAI vendors include experimental header files in the libsai package.

[kcudnik]

@kcudnik , I am able to build RPC image for BCM DNX if i change the sai-thrift Makefile to use the experimental header files from /usr/include/sai/experimental. Not sure if all SAI vendors include experimental header files in the libsai package.

im not sure that ether, but my proposal was to actually point to SAI/inc since experimental is pointed in

CFLAGS = -I$(SAI_HEADER_DIR) -I. -I../../experimental -std=c++11

and the issue is with regular includes that are pulled of from /usr/include/sai, so if we would change that SAI_HEADER_DIR to ../inc during build make then it should be fine

[saksarav-nokia]

@kcudnik , I am able to build RPC image for BCM DNX if i change the sai-thrift Makefile to use the experimental header files from /usr/include/sai/experimental. Not sure if all SAI vendors include experimental header files in the libsai package.

im not sure that ether, but my proposal was to actually point to SAI/inc since experimental is pointed in

CFLAGS = -I$(SAI_HEADER_DIR) -I. -I../../experimental -std=c++11

and the issue is with regular includes that are pulled of from /usr/include/sai, so if we would change that SAI_HEADER_DIR to ../inc during build make then it should be fine

Yes. In our local build, we unblocked the build failure by using ../../inc instead of /usr/include/sai

[kcudnik]

great, did you changed that in Makefile in SAI repo or in build process of this package in build image?

[saksarav-nokia]

i changed in sai-thrift Makefile
saksarav-nokia/SAI@2cbd247

[kcudnik]

yea, but this will limit functionality, will be be able to change the code that is actually calling this Makefile? and pass SAI_HEADER_DIR=../../inc in command line ?

[liushilongbuaa]

@saksarav-nokia , @kcudnik
What's the status now?

[kcudnik]

asked @saksarav-nokia whether he can make change in build #20322 (comment)

[saksarav-nokia]

asked @saksarav-nokia whether he can make change in build #20322 (comment)

Will there be any functionality impact. We have tested with only BCM dnx SAI and it is working fine. How about other SAI vendors.

[kcudnik]

its hard to say on other vendors, originally this code was added by mlnx

[abdosi]

@kcudnik what should be next steps to fix for master ?

[kcudnik]

i proposes solution here: #20322 (comment)

[liushilongbuaa]

@kcudnik , who can do the code change? @saksarav-nokia ?
Maybe we can create a PR for test.

[kcudnik]

guys here already mentioned that they made local change and it works ? so can you raise PR ?

[liushilongbuaa]

i proposes solution here: #20322 (comment)

@kcudnik , so we are waiting for @saksarav-nokia create PR to nokia repo?

[kcudnik]

@saksarav-nokia please create PR if you already have the fix for this

[saksarav-nokia]

@saksarav-nokia please create PR if you already have the fix for this

Yes. I will create the PR

[saksarav-nokia]

@kcudnik , I have created the PR. Pls review it

[kcudnik]

@kcudnik , I have created the PR. Pls review it

Approved, but please provide more explanation in that pr description why this fix is made

[saksarav-nokia]

@kcudnik , I have created the PR. Pls review it

Approved, but please provide more explanation in that pr description why this fix is made

@kcudnik , Added more explanation . Pls check

[kcudnik]

merged