-
Notifications
You must be signed in to change notification settings - Fork 851
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
Implementation of SWIG with C# bindings #2577
base: master
Are you sure you want to change the base?
Conversation
* -add swig bindings and wrapper for c#
-improve some comments in CMakeLists -default win swig back to OFF
@lewk2 |
Documentation - absolute; in the background I am preparing some simple samples (hosted on our github, so can link - or we can dupe over into here later should anyone like to keep things in one place). Can certainly try to get some of the current style 'API docs' added into this PR over the lifecycle of this so that there is a bit more detail. About tidying scripts... I merely stand on top of the somewhat unstable pile of scripts already in place. Tugging at that thread might unweave quite a bit... I don't mind doing such a task (again, a bit in the background as-and-when) - but probably a) outside of this PR and b) doing something like making a new folder structure and trying to migrate over useful things to help see what is really left over as topical. It's something that can be debated a bit - I could also slim a few away from this PR as being not strictly required (I added a few more as a 'show your working' approach so people can see what is what) to try to not make the situation too much worse in the scope of this PR. I would note - dealing with 'script tidying' up not a fun thing to do - and it gets worse if it is a blended task of tidying along with some functional changes - so splitting that task would be almost a dealbreaker if you want me to touch it :) Also, if we agree to let me go a bit nuts cleaning that whole area up (or making a new clean area and migrating things we can identify the purpose of still) it would probably actually make sense to slim this PR (e.g. remove nuget building stuff, remove linux docker stuff) to make this a more focussed PR anyway. In summary, I'll open that can of worms if you want me too - but we're cooking that can before serving it for dinner :) |
Absolutely true! The scripts folder is waiting for a hero to clean it up. As I remember there are also duplicated scripts of building SRT on windows to create an installer and building SRT on Windows for AppVeyor. |
SWIG definition updates
-set execution bit on build_lin script (CI problem)
-typedef byte (needed to compile on linux) -download swig from cinegy s3 link (fix flakey CI)
-add socketID into delegate dictionary name on listen/connect
Merge enhancement from @BlackGad into PR linked branch
-added missed () from cmake else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the bug - set build-lin-docker.ps1
file +x (execution bit). Without this it can't be run through /usr/bin/pwsh
.
Another problem found - unsure why this problem didn't pop up here, but it does pop up in the CI-verification PR #2710 and it is clearly caused by this one:
Note that if you want to use any policy changes, the requested oldest cmake version must be regarded. There is a variety of soluitions here:
|
This was already fixed back in Feb (see 7971f28). Possibly something has glitched somewhere, but the file has +x bit set on my side - and I think CI would be totally broken without this... I just did a new checkout from the 'master-swig' branch in the Cinegy fork to a Mac, and that file came down with that bit set OK. |
This was fixed on the 5th May already (which is why thge CI passes on this PR. Probably it was fixed part-way through the review - and when our internal developer tried to review this requested change it confused him (since it seemed to be fixed already). BTW - I have now left Cinegy (and took some time off - hence slow reply), so I'm answering here as a 'private individual' :) Vova Shkolka (ZTBlackGad in GitHub) is still at Cinegy, and he was the author of the later Swig C# work (and retained that task). However, I'm hoping a re-review of the above will mean everything here has a clean bill of health. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2577 +/- ##
==========================================
- Coverage 67.15% 67.09% -0.07%
==========================================
Files 102 102
Lines 20368 20368
==========================================
- Hits 13678 13665 -13
- Misses 6690 6703 +13 ☔ View full report in Codecov by Sentry. |
This PR shows SWIG inclusion for C#, with clearer type mapping for easy managed code consumption.
Included to permit easier testing is a Linux 'build container' Dockerfile that is used to make it easy to get a consistent build across different CI systems (we use TeamCity in Cinegy).
The required 'nuget' files are left for reference in the scripts folder, that is used by me to push the 'SrtSharp' nuget package to public repos.
VS2013 support on the AppVeyor system is dropped within this PR, since it's nearly 10 years old now and I already added more to it - it's slow enough already :)
Everything SWIG is disabled by default in CMake, and just the tiniest #define is twiddle is added to the srt.h file (most other changes are new files or just scripting stuff for CI code)