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

Remove use of vendor specific macros #1975

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bluecmd
Copy link
Contributor

@bluecmd bluecmd commented Mar 1, 2024

SAI should not care about which vendor is loaded, so remove the compile time requirement to specify which vendor will be.

Proposal from discussions in #1972

@kcudnik
Copy link
Collaborator

kcudnik commented Mar 1, 2024

pleasse add authors of code that is being removed to disscuss whether this is safe

SAI should not care about which vendor is loaded, so remove the compile
time requirement to specify which vendor will be.

Signed-off-by: Christian Svensson <[email protected]>
@bluecmd bluecmd force-pushed the pr-no-vendor-macro branch from 83ced79 to 84ed41d Compare March 1, 2024 12:59
@bluecmd
Copy link
Contributor Author

bluecmd commented Mar 1, 2024

pleasse add authors of code that is being removed to disscuss whether this is safe

Sure, looking at older PRs I guess this is @chrispsommers and @richardyu-ms.

I hope this ping is sufficient to ask for a review :-)

@richardyu-ms
Copy link
Collaborator

richardyu-ms commented May 19, 2024

did you make test with your changes in different platforms, meanwhile did you run those change in build-image repo?
Indeed, we expect the SAI module does not depend on vendors, but when SAI module build on different vendor platform it related to different SAI implementation/driver, then there we might need different build parameters to build SONiC image with different SAI implementations from different vendors

@bluecmd
Copy link
Contributor Author

bluecmd commented May 19, 2024

I have tested this successfully on Broadcom and Innovium SAI.

@richardyu-ms
Copy link
Collaborator

I have tested this successfully on Broadcom and Innovium SAI.

Cause in your changes, there are some changes to remove the build parameters across many platforms

ifeq ($(platform),MLNX)
CDEFS = -DMLNXSAI
else
ifeq ($(platform),BFT)
CDEFS = -DBFTSAI
else
ifeq ($(platform),CAVIUM)
CDEFS = -DCAVIUMSAI
else
CDEFS = -DBRCMSAI
endif
endif
endif

I think it is better to build the image with your code on those platforms at least, it can make sure the sai component will not impact the SONiC image build.

@bluecmd
Copy link
Contributor Author

bluecmd commented May 30, 2024

@richardyu-ms I do not have any possibility of testing all those platforms.

It seems to me that this code is dead and unmaintained, and I would be surprised if anyone with Barefoot or Cavium runs this code.

If you are certain you require testing on these platforms to remove this code, I will be forced to give up on this PR.

[..] it can make sure the sai component will not impact the SONiC image build.

Is the saithrift component really used in SONiC? I thought it was only used internally in SAI for tests.

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