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

Allow unknown fields when parsing P4Info files #4341

Merged

Conversation

jafingerhut
Copy link
Contributor

No description provided.

@jafingerhut
Copy link
Contributor Author

This PR is the same as #4129 except it is up to date with latest main version of p4c.

@jafingerhut
Copy link
Contributor Author

I have checked the protobuf source repo here https://github.com/protocolbuffers/protobuf looking through several versions of it to see which versions support the allow_unknown_field optional parameter to the Python method google.protobuf.text_format.Merge, and which do not.

I believe that this optional parameter was added in v3.8.0 of Protobuf. I would guess that the reason this p4c CI test:

  • test-p4c / test-ubuntu20 (Unity ON)

is failing, is because it is using a version of Protobuf older than that, but I am not sure where to verify that guess.

@fruffy
Copy link
Collaborator

fruffy commented Jan 20, 2024

I have checked the protobuf source repo here https://github.com/protocolbuffers/protobuf looking through several versions of it to see which versions support the allow_unknown_field optional parameter to the Python method google.protobuf.text_format.Merge, and which do not.

I believe that this optional parameter was added in v3.8.0 of Protobuf. I would guess that the reason this p4c CI test:

* test-p4c / test-ubuntu20 (Unity ON)

is failing, is because it is using a version of Protobuf older than that, but I am not sure where to verify that guess.

What happens if you remove this check here? It might be that the preinstalled Protobuf version is too old.

Copy link
Collaborator

@fruffy fruffy left a comment

Choose a reason for hiding this comment

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

Seems like it works. Would be great to add these dependencies to the requirements.txt file. But this might break Ubuntu 18.04, which we still run as a nightly build.

@jafingerhut
Copy link
Contributor Author

Seems like it works. Would be great to add these dependencies to the requirements.txt file. But this might break Ubuntu 18.04, which we still run as a nightly build.

Commit 4 of this PR I added the versions of protobuf, grpcio and googleapis-common-protos packages to requirements.txt. We will see soon from CI tests what happens.

@jafingerhut
Copy link
Contributor Author

@fruffy Where can one find the results of nightly Ubuntu 18.04 builds?

@fruffy
Copy link
Collaborator

fruffy commented Jan 22, 2024

@fruffy Where can one find the results of nightly Ubuntu 18.04 builds?

https://github.com/p4lang/p4c/actions/workflows/ci-ubuntu-18-nightly.yml

The problem is that they will not run on this PR. I think we can change that.

@jafingerhut
Copy link
Contributor Author

Or if we want to take a chance, I can merge this PR and then we'll see tomorrow what the nightly results are, and backo out the requirements.txt changes if they mess those up? I think I will try that now.

@jafingerhut jafingerhut merged commit 870c103 into p4lang:main Jan 22, 2024
13 checks passed
@fruffy
Copy link
Collaborator

fruffy commented Jan 23, 2024

https://github.com/p4lang/p4c/actions/runs/7619068359/job/20753493581

It looks like Ubuntu 18 does not support anything over Protobuf 3.19.

@jafingerhut
Copy link
Contributor Author

@fruffy Any preference on what to do next? e.g. back out the changes to the requirements.txt file?

@fruffy
Copy link
Collaborator

fruffy commented Jan 23, 2024

I created #4348 to run optional CI runs by adding tags. I can try to see whether downgrade to 3.19 can help. Otherwise, we can only remove it from the file since requirements.txt doesn't support optional installation.

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