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

Implementation of SWIG with C# bindings #2577

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

lewk2
Copy link
Contributor

@lewk2 lewk2 commented Dec 8, 2022

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)

lewk2 and others added 4 commits December 6, 2022 17:54
* -add swig bindings and wrapper for c#
* -add swig bindings and wrapper for c#
-improve some comments in CMakeLists
-default win swig back to OFF
@maxsharabayko maxsharabayko added this to the v1.6.0 milestone Dec 8, 2022
@maxsharabayko
Copy link
Collaborator

@lewk2
Thank you for submitting this!
I feel like some documentation is missing, guiding what to do with all those scripts.
Maybe the scripts folder should have a subfolder for this, to avoid mixing with other scripts.

@maxsharabayko maxsharabayko added Type: Enhancement Indicates new feature requests [apps] Area: Test applications related improvements labels Dec 8, 2022
@lewk2
Copy link
Contributor Author

lewk2 commented Dec 9, 2022

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 :)

@maxsharabayko
Copy link
Collaborator

@lewk2

I merely stand on top of the somewhat unstable pile of scripts already in place.

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.

Copy link
Collaborator

@ethouris ethouris left a 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.

@ethouris
Copy link
Collaborator

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:

#SWIG support - adding bindings for other languages into a new library that statically include the main SRT lib
cmake_policy(SET CMP0078 NEW)
cmake_policy(SET CMP0086 NEW)

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:

  • Lift the minimum cmake version - not recommended, consult this with @maxsharabayko, we likely need it to stay as low as possible
  • Block the policy for versions older than those that support it
  • Block the parts that depend on this policy if cmake version is too low

@lewk2
Copy link
Contributor Author

lewk2 commented Jul 15, 2023

Please fix the bug - set build-lin-docker.ps1 file +x (execution bit). Without this it can't be run through /usr/bin/pwsh.

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.

@lewk2
Copy link
Contributor Author

lewk2 commented Jul 15, 2023

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:

#SWIG support - adding bindings for other languages into a new library that statically include the main SRT lib
cmake_policy(SET CMP0078 NEW)
cmake_policy(SET CMP0086 NEW)

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:

* Lift the minimum cmake version - not recommended, consult this with @maxsharabayko, we likely need it to stay as low as possible

* Block the policy for versions older than those that support it

* Block the parts that depend on this policy if cmake version is too low

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-commenter
Copy link

codecov-commenter commented Jan 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (02acd18) 67.15% compared to head (97bd170) 67.09%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

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.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[apps] Area: Test applications related improvements Type: Enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants