mbj4668 / pyang

An extensible YANG validator and converter in python
ISC License
530 stars 342 forks source link

Sid sx structure #839

Closed mcr closed 10 months ago

mcr commented 1 year ago

allocate SID values when an sx:structure is used in a grouping, just like yang-data.

predi commented 1 year ago

@mcr you requested help on core WG mailing list and I presume this is the set of changes you were referring to (or a part of it). Please check https://github.com/predi/pyang/commit/0782596479a4e1b09250836f9660c45fb6c84c2e to see how'd approach this. My fork should properly emit SIDs for all schema nodes, while also generating proper schema node identifiers for them (per ietf-sid-file YANG, core-sid-20). The one thing that does not work is sx:augment-structure. That doesn't seem to be implemented in pyang at all. I have no idea how to run tests, so I didn't bother with those.

mcr commented 1 year ago

Running the tests is easy. Please run the tests so that you know that you aren't breaking things. Your patch set seems to eliminate a lot of code, and I can't really tell if you've just done the job better, or if you've omitted some important functionality. Run the tests. If you want to get onto shared screen(1) with me next week, please unicast me.

predi commented 1 year ago

I know the inner workings of pyang well and also know YANG. The main purpose of the code I removed was essentially to filter out things that didn't really need to be filtered out. pyang cooks/compiles/expands a YANG schema tree by populating the i_children member of statement objects, starting with the module. Each statement object inside i_children will have this same member set if it has schema node children, forming a tree of objects. This tree is what needs to be traversed and each node in it should receive a SID ("data" namespace). You don't need to worry about stuff like groupings, uses, etc. (they don't get to be a part of i_children directly). All implicitly present schema nodes are already in there as well. What does need to be filtered out are nodes contributed by other modules using augmentation.

The tests don't work for me, not on master. I'm on Ubuntu 22.04.1 LTS (GNU/Linux 5.15.90.1-microsoft-standard-WSL2 x86_64), Python 3.10.6, created a venv with all dev requirements plus pyang and lxml. When I cd into "test/test_sid" and run "make test", stuff just errors out, for example "make test5":

# Test augment and yang data
sid-generate-file 2500:50 ietf-constrained-voucher@2019-08-01.yang 2>&1 | diff -b test-5-expected-output.txt -
1,4c1
<
< File ietf-constrained-voucher@2019-08-01.sid created
< Number of SIDs available : 50
< Number of SIDs used : 13
---
> /bin/sh: 1: sid-generate-file: not found
make: [Makefile:27: test5] Error 1 (ignored)
diff -b test-5-expected-ietf-constrained-voucher@2019-08-01.sid ietf-constrained-voucher@2019-08-01.sid
diff: ietf-constrained-voucher@2019-08-01.sid: No such file or directory
make: *** [Makefile:28: test5] Error 2

Not sure what I'm doing wrong and would appreciate some pointers on how to make this work as expected. Running make in parent dirs doesn't make a difference.

mcr commented 1 year ago

jernejt @.***> wrote:

The tests don't work for me, not on master. I'm on Ubuntu 22.04.1 LTS (GNU/Linux 5.15.90.1-microsoft-standard-WSL2 x86_64), Python 3.10.6, created a venv with all dev requirements plus pyang and lxml. When I cd into "test/test_sid" and run "make test", stuff just errors out, for example "make test5":

Yes, run "make tests" from the top. I sent some pull requests to pyang to make it easier, but they don't seem to have been adopted yet.

predi commented 1 year ago

Managed to get individual tests in "test/test_sid" working by invoking make testX PYANG=pyang, where X is the number of the test.

Yeah, I may have broken rt:yang-data (test5). I'm not sure whether only the container within it is supposed to get a SID assigned, or both the container and the enveloping rt:yang-data "node" (this is the main difference compared to sx:structure). core-sid-20 should probably say something about this, but doesn't. Since the new proposal is to "assign a SID to everything" including stuff that does not currently get to be instantiated in instance documents, rt:yang-data node should probably be included in the mapping. This YANG breaks the mapping on master:

module terrible-yang-data {
    yang-version 1.1;
    namespace "terrible-yang-data:uri";
    prefix tyd;

    import ietf-restconf {
        prefix rc;
    }

    rc:yang-data data {
        container payload;
    }

    container payload;
}
mcr commented 10 months ago

@predi I'm perfectly happy for you to write better code, but it's not really better if you don't know what it needs to do. I suggest that you review and approve my changes, just rebased, so that we can make progress. If your way is better, then great.