Conversation
|
Hello @marten-seemann, Thanks for this. As you surmised however, there were indeed still quite a few problems with the first attempt that I tried to solve with my new commit. Primarily, you were indeed not properly checking your CDDL definitions. The CDDL tool doesn't properly validate "unused" rules (only does high-level syntax checking, but no CDDL semantics) which are like... almost all of them :( @lnicco had the same problem in his PR and he eventually solved it by generating stubs for the unused rules with a custom script. I've gone and changed that script minimally to work for the quic draft as well. You can find my version here: https://gist.github.com/rmarx/85b7b86130f8a8dcbe58f3c0d05620ff. Doing that highlighted a few omissions (e.g., no definitions for ConnectionID/Owner/QuicVersion/uint16/...) which I added (or will add in the main schema). It also showed you were not using the correct syntax to create the trigger lists (not using the / concatenation operator). For some reason this is valid overall CDDL syntax, but it generates bogus JSON results and doesn't work well in many situations. I've updated all those as well. I was also able to resolve your 3 TODO's in (I think) a satisfactory way. Please validate my proposed solution for those. Finally, to prevent global CDDL name collisions, we've been adding the category name in front of each event name (so TransportParametersSet instead of just ParametersSet). I've done that for all the events here as well. Similarly, we should add an ID to each CDDL block (e.g., The latest version generates properly with the CDDL tool (using the gist script above) and also passes the id-nits check builtin to the draft make command. To me, this is ready to submit as a new draft by Monday. Please review and let me know if you detect any more issues @marten-seemann @lnicco @LPardue |
This will still need a bit of work, but
succeeds (as long as I insert the definitions for
uint64and forhexstringfrom the main document). I'm not 100% sure if that means that my definitions are ok, as CDDL just tells me that almost all of the rules are unused.There are a few TODOs left (search the doc for TODO), which we probably want to resolve before merging this PR. @rmarx, your opinion on those would be highly appreciated.