Enhance enum checker to include meta header files by tjchadaga · Pull Request #2261 · opencomputeproject/SAI

tjchadaga · 2026-03-10T17:20:28Z

This PR expands the existing enum value lock/ancestry tooling to also evaluate SAI enums defined under the meta/ header set (and additionally runs the lock check against experimental/), helping detect unintended enum value shifts beyond just inc/.

kcudnik · 2026-03-10T20:55:27Z

meta/checkancestry.sh

+#BEGIN_COMMIT=1f2bca1 # to this commit we have history file
+BEGIN_COMMIT=c72635e # from this commit we are backward compatible


please revert, you dont have history file with this commits

there is a file ancestry.1f2bca1.history in meta directory which was generated using ancestry.pl script with -D flag, and this is used to speed up validation in PRs, and first commit must be 3132018, dont change that

but since you are adding meta directory so from 3132018 this commit (2021) there were some incompatible changes in meta

thanks for looking at this, @kcudnik. Yes, I'm trying to update the begin commit to a more recent commit where the meta incompatibility was fixed. This way we can check both inc and meta for incompatibilities going forward. What issues do you see with making this update?

there are enums that were removed, and then added back, since that commit, and validator is checked whether in future there are noconflicts with that, so if you move begin commit to newer one, all previous history will be forgoten

ok, I changed it to exclude around 20 commits where backward compatibility was broken. Hopefully this is a better compromise, since the history is not fully lost

i dont agree on that, i want to preserve all history, and you should add exception in ancestry.pl for metadata header explicytly and explicitly at speific commit, please go this route

made the change to use skipped commits for metadata headers only

tjchadaga · 2026-03-10T21:06:28Z

/azp run

azure-pipelines · 2026-03-10T21:06:37Z

Azure Pipelines successfully started running 1 pipeline(s).

tjchadaga · 2026-03-10T22:20:24Z

/azp run

azure-pipelines · 2026-03-10T22:20:33Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull request overview

This PR expands the existing enum value lock/ancestry tooling to also evaluate SAI enums defined under the meta/ header set (and additionally runs the lock check against experimental/), helping detect unintended enum value shifts beyond just inc/.

Changes:

Update checkheaders.pl to compile enums against inc/sai.h and (when needed) include headers from non-inc directories (e.g., meta/).
Extend checkenumlock.sh to compare enum values for meta/ and experimental/ in addition to inc/.
Extend ancestry tooling (checkancestry.sh, ancestry.pl) to incorporate headers from meta/ and add a commit-range skip mechanism.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
meta/checkheaders.pl	Adjusts include strategy for non-`inc` directories; updates gcc include path.
meta/checkenumlock.sh	Checks enum-lock stability for `meta/` and `experimental/` in addition to `inc/`.
meta/checkancestry.sh	Checks out `meta/` per-commit and adds skip-range filtering to commit list generation.
meta/ancestry.pl	Processes `meta/` headers alongside `inc/` when building enum history.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

meta/checkheaders.pl

tjchadaga · 2026-03-10T22:44:33Z

/azp run

azure-pipelines · 2026-03-10T22:44:41Z

Azure Pipelines successfully started running 1 pipeline(s).

meta/checkancestry.sh

Signed-off-by: Tejaswini Chadaga <tchadaga@microsoft.com>

tjchadaga · 2026-03-12T22:55:59Z

/azp run

azure-pipelines · 2026-03-12T22:56:09Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

meta/ancestry.pl

+    'dir=s'            => \$mydir,       # Directory to process, e.g. inc or meta (default: inc)
+    'commits=s{,}'     => \@mycommits,   # --commits c1 c2 c3 ...
+) or die "Usage: $0 [opts] --dir <dir> --commits <c1> [c2 ...]\n";



meta/checkancestry.sh

+    local SKIP_LIST=$(git rev-list --ancestry-path ${skip_begin}^..${skip_end} | xargs -n 1 git rev-parse --short | tac)
+
+    LIST_INC=$FULL_LIST
+    LIST_META=$(grep -vxFf <(echo "$SKIP_LIST") <<< "$FULL_LIST")


kcudnik · 2026-03-13T06:08:25Z

meta/ancestry.pl

 use Getopt::Std;
 use Data::Dumper;
 use utils;

 my %options = ();



this could be removed @tjchadaga

kcudnik · 2026-03-13T06:08:11Z

meta/checkancestry.sh

+    perl ancestry.pl -H "ancestry.1f2bca1.history" --dir "inc" --commits $LIST_INC
+    perl ancestry.pl -H "ancestry.1f2bca1.history" --dir "meta" --commits $LIST_META


why this is executed twice? since normally history file is based on entire include files, this will basically split it to 2 entries, what i meant in my comment previously to add explicit skip for commits for meta directory inside ancesry.pl file not checkancestry.sh, this will work what you implemented, but it will not generate histroy file for meta directory, with your approach you would need 2 files with histrou, one for inc one for meta

kcudnik · 2026-03-13T06:09:50Z

meta/ancestry.pl

    my $commit = shift;

-    my @headers = GetHeaderFiles("temp/commit-$commit/inc");
+    my $dir = defined($mydir) ? $mydir : 'inc';


is this check done here fir inc/meta switch? why then mydirisadded if that directory is given from command line? should it alwys be command line ? and maybe add t hen check here if the dir dont exists then exit(1) ?

kcudnik · 2026-03-13T06:11:43Z

@tjchadaga remind me again why enum check is done for meta data directory ? since if meta directory are not serialized, and are used only in compilation betweend syncd and sairedis (maybe orchagent? not sure here) so you would get missmatch if you would use differet sairedis library with differetn syncd library which make no sesne to use

pleae provide scenario where you got explicit failure of enum shif in meta headers

tjchadaga force-pushed the saimetachecker_fix branch from d89ea47 to 452ef75 Compare March 10, 2026 20:52

kcudnik reviewed Mar 10, 2026

View reviewed changes

tjchadaga force-pushed the saimetachecker_fix branch from 452ef75 to 3c6b921 Compare March 10, 2026 22:14

tjchadaga requested a review from Copilot March 10, 2026 22:28

Copilot started reviewing on behalf of tjchadaga March 10, 2026 22:29 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

meta/checkheaders.pl Show resolved Hide resolved

meta/checkheaders.pl Outdated Show resolved Hide resolved

meta/checkheaders.pl Show resolved Hide resolved

tjchadaga force-pushed the saimetachecker_fix branch from 3c6b921 to bcdad85 Compare March 10, 2026 22:41

tjchadaga marked this pull request as ready for review March 10, 2026 23:29

tjchadaga changed the title ~~[Draft] Enhance enum checker to include meta header files~~ Enhance enum checker to include meta header files Mar 10, 2026

kcudnik reviewed Mar 11, 2026

View reviewed changes

meta/checkancestry.sh Show resolved Hide resolved

Enhance enum checker to include meta header files

b880b86

Signed-off-by: Tejaswini Chadaga <tchadaga@microsoft.com>

tjchadaga force-pushed the saimetachecker_fix branch from bcdad85 to b880b86 Compare March 12, 2026 22:52

tjchadaga requested review from Copilot and kcudnik March 12, 2026 23:29

Copilot started reviewing on behalf of tjchadaga March 12, 2026 23:44 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

kcudnik reviewed Mar 13, 2026

View reviewed changes

		#BEGIN_COMMIT=1f2bca1 # to this commit we have history file
		BEGIN_COMMIT=c72635e # from this commit we are backward compatible

		perl ancestry.pl -H "ancestry.1f2bca1.history" --dir "inc" --commits $LIST_INC
		perl ancestry.pl -H "ancestry.1f2bca1.history" --dir "meta" --commits $LIST_META

Conversation

tjchadaga commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjchadaga Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjchadaga commented Mar 10, 2026

Uh oh!

azure-pipelines bot commented Mar 10, 2026

Uh oh!

tjchadaga commented Mar 10, 2026

Uh oh!

azure-pipelines bot commented Mar 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tjchadaga commented Mar 10, 2026

Uh oh!

azure-pipelines bot commented Mar 10, 2026

Uh oh!

Uh oh!

tjchadaga commented Mar 12, 2026

Uh oh!

azure-pipelines bot commented Mar 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kcudnik commented Mar 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tjchadaga commented Mar 10, 2026 •

edited

Loading

tjchadaga Mar 10, 2026 •

edited

Loading