WIP: partial fix for issue #386

[mihaibudiu]

@antoninbas : Please see if this solves the problems with action names.
We should discuss whether we have to apply this to other objects besides actions.
This PR can be merged as it is, but if we need to do more then we should keep the issue open.

There was a bug in the testing script which would not check the midend outputs; fixing that uncovered that many reference outputs were wrong, and that p4test was missing an important pass. So these fixes are in this PR as well.

[antoninbas]

Have we removed table parameters yet? Cause it's rejecting a program that I think used to be accepted by the compiler:

table t() { ... }

gives me:

p4_16_dup.p4(43):syntax error, unexpected '(', expecting '{'
    table t(

Regarding the names:

• it seems that the dot remains in the name produced by the P4Runtime serializer, but maybe @sethfowler can take care of this. Apart from that it works fine for P4_14.
• for P4_16, I can see no dot in the names produced by the serializer. However, I still get _<number> appended to my action names; it would be nice if an annotation could be added by the compiler to keep the same name in all action clones.

[mihaibudiu]

In P4-16 are you talking about global actions?

[mihaibudiu]

Indeed, there was an earlier PR which removed table parameters.
We have to discuss in the design group monday whether we remove them, but there is a strong push to do so.
We could change the syntax to accept tables with empty parameter lists too.

[antoninbas]

yes I'm talking about P4_16 global actions. Unless I explicitly give an annotation to the action in the P4 code, the clones get a _<number> suffix,

[mihaibudiu]

I have extended this behavior for P4-16 global actions.

[antoninbas]

Please don't merge anything until the leading dot is stripped by the P4Runtime serializer, maybe @sethfowler can add his commit to this PR.

[sethfowler]

I can add the commit to this PR, yes.

This PR needs either a merge or a rebase. @mbudiu-vmw, do you want me to rebase it?

[sethfowler]

Hmm, actually I doubt I can push to Mihai's repo. I can open a separate PR that includes these commits and also the p4RuntimeSerializer change, though. @mbudiu-vmw, would you like to me to do that?

[mihaibudiu]

I really have no preferences; whatever is easiest for you. You can also submit a separate PR, and merge yours first - it won't have any effects.

[sethfowler]

I can certainly land a patch that just strips any leading dot off of action names first, yeah. I'll put that together.

In fact, it looks to me like the only things that would require more complicated handling would be header fields, and since we aren't exposing those in the control plane API any more, stripping off the leading dot may be all that I have to do.

[ChrisDodd]

Seth could create a pull request into Mihai's repo that Mihai could pull into this pull request, but I don't think he can otherwise chhage this pull request. Alternately, pull Mihai's changes into your repo and create a new pull request to p4c/master.

[sethfowler]

OK, #399 is on master. Let me know if there are any other P4Runtime-related issues that need to be fixed to get this PR landed.

[antoninbas]

Could we go ahead and merge this now that #399 is in master?

[mihaibudiu]

I certainly won't object.

[antoninbas]

I could merge, but it seems there are conflicts in jsonconverter.cpp now

[antoninbas]

@mbudiu-vmw I'd like to be able to use this from master soon, but it seems that one test is failing: p14_to_16/testdata/p4_14_samples/exact_match_mask1.p4

[ChrisDodd]

Looks like just a case of updating the expected output

[mihaibudiu]

Yes, Chris is right.
But I don't understand why this hasn't failed for me while testing.

[ChrisDodd]

Its a new test that you probably haven't pulled

…

On Thu, Mar 30, 2017 at 8:46 AM, Mihai Budiu ***@***.***> wrote: Yes, Chris is right. But I don't understand why this hasn't failed for me while testing. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#395 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD4c8ZSEedDE28Ei9jiMCuUsm7itwS9pks5rq85egaJpZM4Mo3w1> .

[mihaibudiu]

I always rebase before submitting. I should have all the tests too.
Is travis merging this into master before running the PR?

[antoninbas]

@mbudiu-vmw thanks for fixing this

[sethfowler]

FYI: Travis doesn't do any merging or rebasing; it runs exactly the commit you give it.