Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use SWIGPYTHON to improve SWIG for GO wrapper. #714

Merged
merged 8 commits into from
Nov 25, 2022
Merged

Use SWIGPYTHON to improve SWIG for GO wrapper. #714

merged 8 commits into from
Nov 25, 2022

Conversation

ganglyu
Copy link
Contributor

@ganglyu ganglyu commented Nov 22, 2022

Why I did it

Use SWIG to generate GO wrapper for sonic-swss-common, and SWIG report syntax error for "%pythoncode%".
Related lines are only used for python wrapper, so we should use "#ifdef SWIGPYTHON".

How I did it

Use SWIGPYTHON to improve SWIG for python wrapper.

How to verify it

Build succeed and pass all UT.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111

Description for the changelog

Use SWIGPYTHON to improve SWIG, and then SWIG can generate GO wrapper.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@ganglyu ganglyu requested a review from liuh-80 November 22, 2022 07:34
liuh-80
liuh-80 previously approved these changes Nov 22, 2022
@liuh-80 liuh-80 dismissed their stale review November 22, 2022 07:56

Build failed, seems the change not work.

@ganglyu ganglyu requested a review from liuh-80 November 22, 2022 08:59
@ganglyu ganglyu changed the title Use SWIGPYTHON to replace SWIG for python wrapper. Use SWIGPYTHON to improve SWIG for python wrapper. Nov 22, 2022
@qiluo-msft qiluo-msft requested a review from lguohan November 22, 2022 09:13
liuh-80
liuh-80 previously approved these changes Nov 22, 2022
@qiluo-msft
Copy link
Contributor

qiluo-msft commented Nov 22, 2022

class SonicDBConfig

suggest add a minimal go unit test in swss-common repo. The useful test case may be sonicdbconfig init


In reply to: 1323630042


In reply to: 1323630042


Refers to: common/dbconnector.h:38 in f2fb1c0. [](commit_id = f2fb1c0, deletion_comment = False)

Add unit test for SonicDBConfig init.
@@ -0,0 +1,13 @@
%module swsscommon
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to reuse pyext/swsscommon.i? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pyext/swsscommon.i is designed for python, and it has embedded python code:
https://github.com/sonic-net/sonic-swss-common/blob/master/pyext/swsscommon.i#L108
I think use another swig file will be better.

@ganglyu ganglyu changed the title Use SWIGPYTHON to improve SWIG for python wrapper. Use SWIGPYTHON to improve SWIG for go wrapper. Nov 23, 2022
@ganglyu ganglyu changed the title Use SWIGPYTHON to improve SWIG for go wrapper. Use SWIGPYTHON to improve SWIG for GO wrapper. Nov 23, 2022
@@ -138,6 +138,9 @@ jobs:
pytest --cov=. --cov-report=xml
mv coverage.xml tests/coverage.xml
gcovr -r ./ -e ".*/swsscommon_wrap.cpp" --exclude-unreachable-branches --exclude-throw-branches -x --xml-pretty -o coverage.xml
redis-cli FLUSHALL
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redis-cli FLUSHALL

I guess you flush redis for test purpose, so it should be just above make -C goext check line. #Closed

goext/Makefile Outdated
.PHONY: all check clean

all:
-$(LN) -s ../pyext/swsscommon.i swsscommon.i
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why -? #Closed

@ganglyu ganglyu requested a review from qiluo-msft November 23, 2022 22:45
@ganglyu ganglyu requested a review from liuh-80 November 24, 2022 03:09
@ganglyu ganglyu merged commit 1d66080 into sonic-net:master Nov 25, 2022
@qiluo-msft
Copy link
Contributor

Looks good to me.

StormLiangMS pushed a commit to StormLiangMS/sonic-swss-common that referenced this pull request Dec 11, 2022
Why I did it
Use SWIG to generate GO wrapper for sonic-swss-common, and SWIG report syntax error for "%pythoncode%".
Related lines are only used for python wrapper, so we should use "#ifdef SWIGPYTHON".

How I did it
Use SWIGPYTHON to improve SWIG for python wrapper.

How to verify it
Build succeed and pass all UT.
StormLiangMS added a commit that referenced this pull request Dec 11, 2022
Cherry-pick from master branch

f312634 - Install swsscommon.i with libswsscommon-dev (Install swsscommon.i with libswsscommon-dev #717) (6 minutes ago) [ganglv]
61b7888 - Use SWIGPYTHON to improve SWIG for GO wrapper. (Use SWIGPYTHON to improve SWIG for GO wrapper. #714) (6 minutes ago) [ganglv]
1c10dac - Add decorator for Yang default value. (Add decorator for Yang default value.  #713) (6 minutes ago) [Hua Liu]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants