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

[sai-gen] Deprecate name attribute for all match keys and action parameters, remove type guessing heuristics on object parent names. #480

Merged
merged 8 commits into from
Dec 13, 2023

Conversation

r12f
Copy link
Collaborator

@r12f r12f commented Dec 13, 2023

This change is based on #479 , hence including all changes in the PR.

No update on SAI headers after this change:

r12f@r12f-dl380:~/data/code/sonic/DASH/dash-pipeline
$ diff SAI/SAI/experimental/ ~/data/code/sonic/DASH-exp/dash-pipeline/SAI/SAI/experimental/

2 comments are updated in lib file, but looks like the previous result is wrong:
image

Take route_vnet as an example, it indeed only take 4 parameters, as ip_is_v6 field doesn't count:
image

@r12f
Copy link
Collaborator Author

r12f commented Dec 13, 2023

Please ignore this PR for review until #479 is checked in.

@r12f
Copy link
Collaborator Author

r12f commented Dec 13, 2023

looks like in certain cases, @name is required: (@chrispsommers as FYI)

image

@chrispsommers
Copy link
Collaborator

looks like in certain cases, @name is required: (@chrispsommers as FYI)

That's fine, there's nothing wrong with the @name annotation, it is widely used and guides P4 artifact generation. The only objection in the past was using the pipe symbol to signify SAI code generation; annotations are preferred.

Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this contribution.

@r12f
Copy link
Collaborator Author

r12f commented Dec 13, 2023

Rebased on top of #479 without any code changes.

@r12f
Copy link
Collaborator Author

r12f commented Dec 13, 2023

looks like in certain cases, @name is required: (@chrispsommers as FYI)

That's fine, there's nothing wrong with the @name annotation, it is widely used and guides P4 artifact generation. The only objection in the past was using the pipe symbol to signify SAI code generation; annotations are preferred.

Yep, makes sense!

@r12f r12f merged commit 0841490 into sonic-net:main Dec 13, 2023
5 checks passed
@r12f r12f deleted the user/r12f/params branch December 13, 2023 17:32
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.

2 participants