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

Deprecate .txt, support in favour of .txtpb. #4352

Merged
merged 3 commits into from
Jan 24, 2024
Merged

Deprecate .txt, support in favour of .txtpb. #4352

merged 3 commits into from
Jan 24, 2024

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Jan 23, 2024

Ran into this so might as well fix it.

Fixes #4066.

First commit has the actual changes, second commit updates the reference files, third commit updates the DPDK and PSA back ends, too.

@fruffy fruffy force-pushed the fruffy/txtpb branch 3 times, most recently from d609bf3 to 99c774c Compare January 23, 2024 22:08
@jafingerhut
Copy link
Contributor

Is it intentional to delete all *.p4 file in the directory testdata/p4_16_bmv_errors, and a few other directories?

@fruffy
Copy link
Collaborator Author

fruffy commented Jan 23, 2024

Is it intentional to delete all *.p4 file in the directory testdata/p4_16_bmv_errors, and a few other directories?

Considering the tests pass, yes. It looks like these are stale error files which have never been cleaned up.

@fruffy fruffy marked this pull request as ready for review January 23, 2024 22:49
@jafingerhut
Copy link
Contributor

Is it intentional to delete all *.p4 file in the directory testdata/p4_16_bmv_errors, and a few other directories?

Considering the tests pass, yes. It looks like these are stale error files which have never been cleaned up.

The reason I ask is that some files with suffix .p4 are source files for unit tests, so deleting those files would eliminate those unit tests, and not cause any test failures.

There are other files with suffix .p4 that are expected output files to be checked against actual output files during tests, and deleting those (without also deleting the corresponding source .p4 files) would case test failures.

I am not sure which category the ones in directory testdata/p4_16_bmv_errors fall under.

@smolkaj
Copy link
Member

smolkaj commented Jan 23, 2024

This is a really nice quality of life improvement, thanks for the change!

@fruffy
Copy link
Collaborator Author

fruffy commented Jan 23, 2024

Is it intentional to delete all *.p4 file in the directory testdata/p4_16_bmv_errors, and a few other directories?

Considering the tests pass, yes. It looks like these are stale error files which have never been cleaned up.

The reason I ask is that some files with suffix .p4 are source files for unit tests, so deleting those files would eliminate those unit tests, and not cause any test failures.

There are other files with suffix .p4 that are expected output files to be checked against actual output files during tests, and deleting those (without also deleting the corresponding source .p4 files) would case test failures.

I am not sure which category the ones in directory testdata/p4_16_bmv_errors fall under.

Oh I see what you mean. Long day, will fix this.

@fruffy fruffy force-pushed the fruffy/txtpb branch 4 times, most recently from fe60ab0 to 2b1445c Compare January 24, 2024 00:44
@fruffy fruffy requested a review from usha1830 January 24, 2024 14:58
Copy link
Contributor

@jafingerhut jafingerhut left a comment

Choose a reason for hiding this comment

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

After commit 3 on this PR I see many changes files in testdata subdirectories with _outputs in their names, and only in those directories, which makes me much more confident that no test cases have been changed or removed. Looks good to me. Given the number of places you found that .txt needed updating to .txtpb, I wouldn't be surprised if there might be another one in there lurking somewhere, but if so, it probably isn't being tested in CI.

@fruffy fruffy merged commit 5082fe3 into main Jan 24, 2024
16 checks passed
@fruffy fruffy deleted the fruffy/txtpb branch January 24, 2024 17:31
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.

Support .textproto extension for output files in Protobuf Text Format
3 participants