libyang
libyang copied to clipboard
CESNET
XPath: deref(): improve error messages
While the error message was accurate, it was not as helpful as it could be. Before:
libyang warn: Argument #1 of xpath_deref is a list node "amplifier".
Now:
libyang warn: Argument #1 of deref() is a list node "/tip-photonic-equipment:amplifier". Expected "leafref" or "instance-identifier".
Internally, the error handling can be simplified because we are now printing the exact same thing in both cases. The user probably doesn't care whether deref() hit a schema non-terminal or a terminal which is of a wrong type anyway.
Unfortunately, it seems that libyang doesn't track the source location of these XPaths. That's a pity especially in case of yanglint where other tools, such as pyang, can provide a file name + line number. That makes it much, much easier to map error messages to source code locations. And since this function does not have access to the outer enclosing element, we cannot print its XPath either.
Fixes #1710
I'm not sure what the CI failure is about -- is that a coding style issue, perhaps? Too bad uncrustify doesn't print out something betetr than FAIL: src/xpath.c (Difference at byte 119182).
The user probably doesn't care whether deref() hit a schema non-terminal or a terminal which is of a wrong type anyway.
Adds more details, nothing else.
it seems that libyang doesn't track the source location of these XPaths.
No, because these messages are generated when processing the compiled module. The line is tracked only when parsing and it does not seem worth it to copy it to the compiled module for all the possible errors and statements. Compiled module prints context node instead that should uniquely identify the problematic statement.
Now as for the PR, all these messages are unified in their format (that is why, for instance, the function name is printed as it can be copied to all the XPath functions). I would like to keep this to some reasonable extent. So if you are not willing to put the exact function name into all the XPath function errors, please keep the __func__ instead, I think it makes little difference anyway. But the conditions can be joined into a single message I suppose.
Regarding uncrustify, if you have version 0.71, you can format it yourself. Otherwise I can adjust that, no problem.
Continuous Integration Result: FAILED
See below for issues. CI System Testrun URL: https://ci1.netdef.org/browse/LIBYANG-LYPULLREQ-268/
This is a comment from an automated CI system. For questions and feedback in regards to this CI system, please feel free to email Martin Winter - mwinter (at) opensourcerouting.org.
Get source / Pull Request: Successful
Building stage: Failed
CentOS 8 amd64: Failed (click for details)
Build RPM Package failed for CentOS 8 amd64: (see full Build RPM Package log at https://ci1.netdef.org/browse/LIBYANG-LYPULLREQ-268/artifact/CENTOS8BUILD/ErrorLog/log_build-rpm.txt)
No tests were found!!!
+ exit 0
error: Installed (but unpackaged) file(s) found:
CentOS 7 amd64: Failed (click for details)
Build RPM Package failed for CentOS 7 amd64: (see full Build RPM Package log at https://ci1.netdef.org/browse/LIBYANG-LYPULLREQ-268/artifact/CENTOS7AMD64/ErrorLog/log_build-rpm.txt)
No tests were found!!!
+ exit 0
error: Installed (but unpackaged) file(s) found:
Fedora 31 amd64: Failed (click for details)
Build RPM Package failed for Fedora 31 amd64: (see full Build RPM Package log at https://ci1.netdef.org/browse/LIBYANG-LYPULLREQ-268/artifact/FEDORA31AMD64/ErrorLog/log_build-rpm.txt)
++ jobs -p
+ exit 0
error: Installed (but unpackaged) file(s) found:
Fedora 29 amd64: Failed (click for details)
Build RPM Package failed for Fedora 29 amd64: (see full Build RPM Package log at https://ci1.netdef.org/browse/LIBYANG-LYPULLREQ-268/artifact/FEDORA29AMD64/ErrorLog/log_build-rpm.txt)
No tests were found!!!
+ exit 0
error: Installed (but unpackaged) file(s) found:
Successful on other platforms/tests
- FreeBSD 12 amd64
- Debian 9 amd64
- NetBSD 8 amd64
- Ubuntu 16.04 i386
- Debian 9 arm8
- Ubuntu 16.04 amd64
- Ubuntu 20.04 arm8
- Debian 9 arm7
- Ubuntu 16.04 arm8
- Ubuntu 18.04 arm7
- Ubuntu 16.04 arm7
- Ubuntu 18.04 ppc64le
- Ubuntu 20.04 amd64
- OpenBSD 6 amd64
- Ubuntu 18.04 arm8
- FreeBSD 11 amd64
- Ubuntu 18.04 amd64
- Debian 10 amd64
it seems that libyang doesn't track the source location of these XPaths.
No, because these messages are generated when processing the compiled module.
It's a super-useful feature when debugging a model with the same XPath condition at several locations. I realize that it is not possible yet, but perhaps the non-compiled schema nodes can be extended with that (and yanglint should use that magic flag to keep these uncompiled nodes around in the context via that private field). Just a suggestion for some future work when you have time, I'm not volunteering here :).
Now as for the PR, all these messages are unified in their format (that is why, for instance, the function name is printed as it can be copied to all the XPath functions). I would like to keep this to some reasonable extent. So if you are not willing to put the exact function name into all the XPath function errors, please keep the
__func__instead, I think it makes little difference anyway. But the conditions can be joined into a single message I suppose.
This is a request from comments from my point. I think that the error message is easier to understand to someone who has not written a YANG library. If you like it, I can fix all other functions in this file using the same pattern for consistency. I can also put the phrase "XPath" somewhere in the output if you think that's even better for the end user. I would like to use function names that come from a standard instead of a libyang-specific name that is slightly different.
This is a request for comments from my point.
I see. Then I have nothing against improving it, I wrote the messages to contain the relevant information to be able to fix the problem while keeping them unified and as simple to copy over to other functions as possible. But like you said, I have a different perspective on what is a clear-enough message so I will appriacate if someone takes the time to improve this. It did not seem worth it to me when writing this code.
BTW, I think uncrustify does not like that on 3773 there is one too many spaces in the indent.