sonic-mgmt-common: port to libyang3 #157

bradh352 · 2025-02-19T15:18:30Z

SONiC Libyang3 upgrade Tracking ticket sonic-net/sonic-buildimage#22385

This PR depends on the other PRs mentioned in the above tracking ticket.

The API changes between libyang1 and libyang3 are drastic.

The cvl test cases are currently passing all tests.

This depends on libyang3's devel branch plus this rejected patch:

validation: Add LYD_VALIDATE_NOEXTDEPS to bypass leafref/when/must CESNET/libyang#2362 -- will never be accepted upstream. libyang maintainer says sonic-yang-mgmt needs to be rearchitected, it is a bad application design to need such a flag.

Or the libyang3 package built via sonic-buildimage which is based on v3.7.8 + all the necessary patches, part of sonic-net/sonic-buildimage#21679

Fixes sonic-net/sonic-buildimage#22385

mssonicbld · 2025-02-19T15:18:33Z

/azp run

azure-pipelines · 2025-02-19T15:18:42Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-02-19T15:56:59Z

/azp run

azure-pipelines · 2025-02-19T15:57:09Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-02-19T17:16:19Z

/azp run

azure-pipelines · 2025-02-19T17:16:28Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-02-19T17:17:24Z

/azp run

azure-pipelines · 2025-02-19T17:17:34Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-02-19T17:20:24Z

/azp run

azure-pipelines · 2025-02-19T17:20:33Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-02-19T17:34:23Z

/azp run

azure-pipelines · 2025-02-19T17:34:32Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-02-19T17:35:27Z

/azp run

azure-pipelines · 2025-02-19T17:35:37Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-02-19T17:37:49Z

/azp run

azure-pipelines · 2025-02-19T17:38:01Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-02-19T17:38:12Z

/azp run

azure-pipelines · 2025-02-19T17:38:33Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-02-19T17:44:29Z

/azp run

azure-pipelines · 2025-02-19T17:44:38Z

Azure Pipelines successfully started running 1 pipeline(s).

bradh352 · 2025-02-19T17:54:48Z

@dutta-partha probably going to need your help on this as you are the original author. I've got the port to libyang3 compiling at least, but I'm not entirely sure how to test it or debug it. I'm sure there are issues here.

@ganglyu I've seen you make some commits here too, maybe you can help?

I think out of all the libyang3 porting this is the one that is the most complex and that I know the least about.

ganglyu · 2025-02-19T23:50:22Z

@bradh352
This pipeline downloads libyang*.deb from the sonic-buildimage.vs master pipeline artifacts. However, since your changes have not been merged into the master branch, the libyang*3.*.deb file is not available.

    - task: DownloadPipelineArtifact@2
      inputs:
        source: specific
        project: build
        pipeline: 142
        artifact: sonic-buildimage.vs
        runVersion: 'latestFromBranch'
        runBranch: 'refs/heads/$(BUILD_BRANCH)'
        patterns: |
            target/debs/bookworm/libyang*.deb
            target/python-wheels/bookworm/sonic_yang_models*.whl
      displayName: "Download sonic buildimage"

ganglyu · 2025-02-19T23:51:33Z

You can follow the pipeline to setup your environment and run unit test.
https://github.com/sonic-net/sonic-mgmt-common/blob/master/azure-pipelines.yml

bradh352 · 2025-02-19T23:54:04Z

@ganglyu

@bradh352 This pipeline downloads libyang*.deb from the sonic-buildimage.vs master pipeline artifacts. However, since your changes have not been merged into the master branch, the libyang_3._.deb file is not available.

Right, I'm aware of that. This PR is expected to fail in the SONiC CI system (azure pipelines) until the dependent PRs are merged.

I was hoping someone more familiar with sonic-mgmt-common could help with testing and validation of this particular PR. Its by far the most complex port to libyang3 out of all the various sub-projects.

ganglyu · 2025-02-20T00:05:06Z

@sachinholla would you please help?

azure-pipelines.yml

mssonicbld · 2025-03-03T23:48:14Z

/azp run

azure-pipelines · 2025-03-03T23:48:22Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-03-04T00:03:02Z

/azp run

azure-pipelines · 2025-03-04T00:03:11Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-03-04T00:06:22Z

/azp run

azure-pipelines · 2025-03-04T00:06:31Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-03-04T00:07:36Z

/azp run

azure-pipelines · 2025-03-04T00:08:00Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-03-04T00:09:48Z

/azp run

azure-pipelines · 2025-03-04T00:09:56Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-03-04T00:12:02Z

/azp run

azure-pipelines · 2025-03-04T00:12:10Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-03-04T00:13:49Z

/azp run

azure-pipelines · 2025-03-04T00:13:58Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-03-04T00:15:02Z

/azp run

azure-pipelines · 2025-03-04T00:15:11Z

Azure Pipelines successfully started running 1 pipeline(s).

sachinholla

Hi @bradh352, all tests are passing on my development vm too.. Changes look good to me. Will ask cvl maintainers to take a look.

Seeing a few compiler warnings though:

In file included from _cgo_export.c:4:
util.go: In function ‘customLogCb’:
util.go:30:65: warning: passing argument 3 of ‘customLogCallback’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
util.go:26:62: note: expected ‘char *’ but argument is of type ‘const char *’
# github.com/Azure/sonic-mgmt-common/cvl/internal/util
../cvl/internal/util/util.go: In function ‘customLogCb’:
../cvl/internal/util/util.go:30:72: warning: passing argument 3 of ‘customLogCallback’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
   30 |         customLogCallback(level, (char*)msg, (char*)data_path?data_path:schema_path);
      |                                              ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
../cvl/internal/util/util.go:26:62: note: expected ‘char *’ but argument is of type ‘const char *’
   26 | extern void customLogCallback(LY_LOG_LEVEL, char* msg, char* path);
      |                                                        ~~~~~~^~~~

cvl/cvl_test.go

bradh352 · 2025-03-04T12:04:22Z

Hi @bradh352, all tests are passing on my development vm too.. Changes look good to me. Will ask cvl maintainers to take a look.

Seeing a few compiler warnings though:

In file included from _cgo_export.c:4:
util.go: In function ‘customLogCb’:
util.go:30:65: warning: passing argument 3 of ‘customLogCallback’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
util.go:26:62: note: expected ‘char *’ but argument is of type ‘const char *’
# github.com/Azure/sonic-mgmt-common/cvl/internal/util
../cvl/internal/util/util.go: In function ‘customLogCb’:
../cvl/internal/util/util.go:30:72: warning: passing argument 3 of ‘customLogCallback’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
   30 |         customLogCallback(level, (char*)msg, (char*)data_path?data_path:schema_path);
      |                                              ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
../cvl/internal/util/util.go:26:62: note: expected ‘char *’ but argument is of type ‘const char *’
   26 | extern void customLogCallback(LY_LOG_LEVEL, char* msg, char* path);
      |                                                        ~~~~~~^~~~

These warnings are from the original code and not something newly introduced. Its due to the Golang<->C (cgo) stuff, where Go doesn't know about the const qualifier in C. If you really want me to address it I can probably look into it, here's a reference to the issue:
https://stackoverflow.com/questions/32938558/cgo-cant-find-way-to-use-callbacks-with-const-char-argument

bradh352 · 2025-03-06T10:49:35Z

@sachinholla any other comments on this review? If not, could you approve it?

faraazbrcm · 2025-03-06T10:58:26Z

@bradh352 Code changes look good to me.
I wanted to ensure SET/GET going fine using gNMI and RESTCONF interfaces. I will run some some tests next week and update you.

bradh352 · 2025-03-06T11:55:44Z

let me know if you need an image to test and for what platform. I've got a tree with all 17 PRs in this series that I test with here: https://github.com/bradh352/sonic-buildimage/commits/bradh352/libyang3/

sachinholla · 2025-03-24T07:12:49Z

In file included from _cgo_export.c:4:
util.go: In function ‘customLogCb’:
util.go:30:65: warning: passing argument 3 of ‘customLogCallback’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
util.go:26:62: note: expected ‘char *’ but argument is of type ‘const char *’
# github.com/Azure/sonic-mgmt-common/cvl/internal/util
../cvl/internal/util/util.go: In function ‘customLogCb’:
../cvl/internal/util/util.go:30:72: warning: passing argument 3 of ‘customLogCallback’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
   30 |         customLogCallback(level, (char*)msg, (char*)data_path?data_path:schema_path);
      |                                              ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
../cvl/internal/util/util.go:26:62: note: expected ‘char *’ but argument is of type ‘const char *’
   26 | extern void customLogCallback(LY_LOG_LEVEL, char* msg, char* path);
      |                                                        ~~~~~~^~~~

These warnings are from the original code and not something newly introduced. Its due to the Golang<->C (cgo) stuff, where Go doesn't know about the const qualifier in C.

@bradh352 -- i don't see this warning with old code. Let us fix it as it shows up during compilation & test runs of all other cvl dependent components. Cgo handles const qualifier if you define a typedef with it. Following works for me:

diff --git a/cvl/internal/util/util.go b/cvl/internal/util/util.go
index a1664f2..9c357b8 100644
--- a/cvl/internal/util/util.go
+++ b/cvl/internal/util/util.go
@@ -23,7 +23,8 @@ package util
 #cgo LDFLAGS: -lyang
 #include <libyang/libyang.h>

-extern void customLogCallback(LY_LOG_LEVEL, char* msg, char* path);
+typedef const char const_char_t;
+extern void customLogCallback(LY_LOG_LEVEL, const_char_t* msg, const_char_t* path);

 static void customLogCb(LY_LOG_LEVEL level, const char* msg, const char* data_path, const char * schema_path, uint64_t line) {
        (void)line;
@@ -188,7 +189,7 @@ func IsTraceSet() bool {
 changing libyang's global log setting */

 //export customLogCallback
-func customLogCallback(level C.LY_LOG_LEVEL, msg *C.char, path *C.char) {
+func customLogCallback(level C.LY_LOG_LEVEL, msg *C.const_char_t, path *C.const_char_t) {
        if level == C.LY_LLERR {
                CVL_LEVEL_LOG(WARNING, "[libyang Error] %s (path: %s)", C.GoString(msg), C.GoString(path))
        } else {

bradh352 · 2025-03-27T12:23:21Z

In file included from _cgo_export.c:4:
util.go: In function ‘customLogCb’:
util.go:30:65: warning: passing argument 3 of ‘customLogCallback’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
util.go:26:62: note: expected ‘char *’ but argument is of type ‘const char *’
# github.com/Azure/sonic-mgmt-common/cvl/internal/util
../cvl/internal/util/util.go: In function ‘customLogCb’:
../cvl/internal/util/util.go:30:72: warning: passing argument 3 of ‘customLogCallback’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
   30 |         customLogCallback(level, (char*)msg, (char*)data_path?data_path:schema_path);
      |                                              ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
../cvl/internal/util/util.go:26:62: note: expected ‘char *’ but argument is of type ‘const char *’
   26 | extern void customLogCallback(LY_LOG_LEVEL, char* msg, char* path);
      |                                                        ~~~~~~^~~~

These warnings are from the original code and not something newly introduced. Its due to the Golang<->C (cgo) stuff, where Go doesn't know about the const qualifier in C.

@bradh352 -- i don't see this warning with old code. Let us fix it as it shows up during compilation & test runs of all other cvl dependent components. Cgo handles const qualifier if you define a typedef with it. Following works for me:

diff --git a/cvl/internal/util/util.go b/cvl/internal/util/util.go
index a1664f2..9c357b8 100644
--- a/cvl/internal/util/util.go
+++ b/cvl/internal/util/util.go
@@ -23,7 +23,8 @@ package util
 #cgo LDFLAGS: -lyang
 #include <libyang/libyang.h>

-extern void customLogCallback(LY_LOG_LEVEL, char* msg, char* path);
+typedef const char const_char_t;
+extern void customLogCallback(LY_LOG_LEVEL, const_char_t* msg, const_char_t* path);

 static void customLogCb(LY_LOG_LEVEL level, const char* msg, const char* data_path, const char * schema_path, uint64_t line) {
        (void)line;
@@ -188,7 +189,7 @@ func IsTraceSet() bool {
 changing libyang's global log setting */

 //export customLogCallback
-func customLogCallback(level C.LY_LOG_LEVEL, msg *C.char, path *C.char) {
+func customLogCallback(level C.LY_LOG_LEVEL, msg *C.const_char_t, path *C.const_char_t) {
        if level == C.LY_LLERR {
                CVL_LEVEL_LOG(WARNING, "[libyang Error] %s (path: %s)", C.GoString(msg), C.GoString(path))
        } else {

Thanks for letting me know how to resolve that with cgo, i'll go ahead and apply that patch. Traveling at the moment, but I'll get to it in the next day or two.

bradh352 marked this pull request as draft February 19, 2025 15:18

bradh352 mentioned this pull request Feb 19, 2025

libyang: upgrade to v3 step 1 -- import libyang3 / libyang3-py3, remove stale versions sonic-net/sonic-buildimage#21679

Merged

bradh352 force-pushed the bradh352/libyang3 branch from 1912d28 to 2af318b Compare February 19, 2025 17:16

bradh352 force-pushed the bradh352/libyang3 branch from 74f0752 to 73b87a5 Compare February 19, 2025 17:44

sachinholla reviewed Feb 20, 2025

View reviewed changes

azure-pipelines.yml Show resolved Hide resolved

bradh352 force-pushed the bradh352/libyang3 branch from 02ce756 to 4a72c1f Compare March 4, 2025 00:09

bradh352 force-pushed the bradh352/libyang3 branch from 4a72c1f to 3fad970 Compare March 4, 2025 00:11

bradh352 force-pushed the bradh352/libyang3 branch from 3fad970 to 36e287b Compare March 4, 2025 00:13

fix test cases

10f9078

bradh352 force-pushed the bradh352/libyang3 branch from 36e287b to 10f9078 Compare March 4, 2025 00:15

bradh352 marked this pull request as ready for review March 4, 2025 00:18

sachinholla reviewed Mar 4, 2025

View reviewed changes

cvl/cvl_test.go Show resolved Hide resolved

bradh352 mentioned this pull request Apr 20, 2025

Enhancement: Upgrade to libyang3 sonic-net/sonic-buildimage#22385

Open

sonic-mgmt-common: port to libyang3 #157

Are you sure you want to change the base?

sonic-mgmt-common: port to libyang3 #157

Uh oh!

Conversation

bradh352 commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mssonicbld commented Feb 19, 2025

Uh oh!

azure-pipelines bot commented Feb 19, 2025

Uh oh!

mssonicbld commented Feb 19, 2025

Uh oh!

azure-pipelines bot commented Feb 19, 2025

Uh oh!

mssonicbld commented Feb 19, 2025

Uh oh!

azure-pipelines bot commented Feb 19, 2025

Uh oh!

mssonicbld commented Feb 19, 2025

Uh oh!

azure-pipelines bot commented Feb 19, 2025

Uh oh!

mssonicbld commented Feb 19, 2025

Uh oh!

azure-pipelines bot commented Feb 19, 2025

Uh oh!

mssonicbld commented Feb 19, 2025

Uh oh!

azure-pipelines bot commented Feb 19, 2025

Uh oh!

mssonicbld commented Feb 19, 2025

Uh oh!

azure-pipelines bot commented Feb 19, 2025

Uh oh!

mssonicbld commented Feb 19, 2025

Uh oh!

azure-pipelines bot commented Feb 19, 2025

Uh oh!

mssonicbld commented Feb 19, 2025

Uh oh!

azure-pipelines bot commented Feb 19, 2025

Uh oh!

mssonicbld commented Feb 19, 2025

Uh oh!

azure-pipelines bot commented Feb 19, 2025

Uh oh!

bradh352 commented Feb 19, 2025

Uh oh!

ganglyu commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ganglyu commented Feb 19, 2025

Uh oh!

bradh352 commented Feb 19, 2025

Uh oh!

ganglyu commented Feb 20, 2025

Uh oh!

Uh oh!

mssonicbld commented Mar 3, 2025

Uh oh!

azure-pipelines bot commented Mar 3, 2025

Uh oh!

mssonicbld commented Mar 4, 2025

Uh oh!

azure-pipelines bot commented Mar 4, 2025

Uh oh!

mssonicbld commented Mar 4, 2025

Uh oh!

azure-pipelines bot commented Mar 4, 2025

Uh oh!

mssonicbld commented Mar 4, 2025

Uh oh!

azure-pipelines bot commented Mar 4, 2025

Uh oh!

mssonicbld commented Mar 4, 2025

Uh oh!

azure-pipelines bot commented Mar 4, 2025

Uh oh!

bradh352 commented Feb 19, 2025 •

edited

Loading

ganglyu commented Feb 19, 2025 •

edited

Loading

faraazbrcm commented Mar 6, 2025 •

edited

Loading