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

Generate initial library bindings using ClangSharp #3

Merged
merged 8 commits into from Aug 14, 2019

Conversation

@FiniteReality
Copy link
Contributor

commented Jul 6, 2019

Some minor tweaks have been made to get this to work, but it should be fine. Currently there are no unit tests - I'm looking into ideas for the best way to implement this.

This is what I have so far in terms of ideas for unit testing:

  • Checking library versions (Might be hard to check - likely best to just check for no exceptions and valid UTF8 data)
  • Connecting using various mainloop impls (threaded, standard, and possibly a custom one to test custom select impls)
  • Checking error messages and their string variants

Anything more than that would be hard since it would be system-dependent, and rely on specific sound hardware to be detected.

I believe you wanted me to include the LGPL copyright notice in the PulseAudio bindings files too, as libpulse is LGPL licensed and these bindings are based on the LGPL code? I haven't included them yet because I wasn't sure which would be best.

Generate initial library bindings using ClangSharp
Some minor tweaks have been made to get this to work, but it should
be fine.

No unit tests as of yet.
@FiniteReality

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

@tannergooding Could you take a look at the generated bindings when you have free time? I only glanced through them so I'm sure I missed something after I generated them.

@tannergooding

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

Overall looks pretty good.

It might be worth checking if there are differences when generating for --config unix-types vs --config windows-types (should generally just be usages of long, unsigned long, and wchar_t). In which case there will be portability issues that need to be thought about.

For unit tests, I've largely been validating things like: "Is this the right StructLayout kind", "Is this blittable", and "Is this the expected size".

There are some functional tests, such as validating mainloop/etc works, that could also be added of course, but I won't block this being merged since testing interop code in a unit test is hard.

@FiniteReality

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

I've regenerated the files with the correct headers and such.

As for the unix-types vs windows types, I don't think that will be an issue since PulseAudio is intended for the UNIXes, and Windows is unsupported. I'll look into those unit tests soon, after I have a poke around the other unit tests to see if I can get some inspiration.

@tannergooding

This comment has been minimized.

Copy link
Collaborator

commented Aug 11, 2019

As for the unix-types vs windows types, I don't think that will be an issue since PulseAudio is intended for the UNIXes, and Windows is unsupported

That seems reasonable. I see that there is some level of Windows support that exists, but not everything works (and like most GNU ports, it is buggy due to platform differences that aren't handled properly).


namespace TerraFX.Interop
{
//[NativeTypeName("unsigned int")]

This comment has been minimized.

Copy link
@tannergooding

tannergooding Aug 11, 2019

Collaborator

Commented out because the attribute doesn't work for enums, right?

This comment has been minimized.

Copy link
@FiniteReality

FiniteReality Aug 11, 2019

Author Contributor

Yeah. I decided to leave it in case we change NativeTypeNameAttribute, so reverting this becomes easier

This comment has been minimized.

Copy link
@tannergooding

tannergooding Aug 11, 2019

Collaborator

Sounds good. I'll fixup the utilities library now (working on getting the new package feed up for automated nightlies).

namespace TerraFX.Interop
{
[UnmanagedFunctionPointer(CallingConvention.Cdecl)]
public unsafe delegate void pa_context_notify_cb_t([NativeTypeName("pa_context *")] pa_context* c, [NativeTypeName("void *")] void* userdata);

This comment has been minimized.

Copy link
@tannergooding

tannergooding Aug 11, 2019

Collaborator

note to self: I really need to fix ClangSharp to normalize things like pa_context *, so that it compares equal with pa_context*

@tannergooding

This comment has been minimized.

Copy link
Collaborator

commented Aug 11, 2019

I'll look into those unit tests soon, after I have a poke around the other unit tests to see if I can get some inspiration.

Given the complexity of unit testing Interop code and that most of the functionality just comes down to asserting attributes and sizes, I'd be fine with checking this in if you add a small sample app to https://github.com/terrafx/terrafx/tree/master/samples/TerraFX.

Just remove the WIP label from this PR whenever you also feel it's ready to merge

Add a simple unit test for testing lib versions
By testing something simple like this, we can roughly assume that
a majority of the P/Invoke code is correct (calling convention,
marshalling, entry points etc.) leaving further testing to further
up in the chain.

@FiniteReality FiniteReality changed the title [WIP] Generate initial library bindings using ClangSharp Generate initial library bindings using ClangSharp Aug 14, 2019

@FiniteReality FiniteReality marked this pull request as ready for review Aug 14, 2019

@FiniteReality

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Oh dear. Looks like I forgot to check whether libpulse was actually available on the CI instances 🙃

@tannergooding

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2019

Looks like I forgot to check whether libpulse was actually available on the CI instances

We can "fix" this by:

  • Skipping the tests if libpulse isn't on the machine (with an appropriate warning/message)
    • Probably the least desirable. Also gives false positives that may be hard to track down
  • Updating the yml scripts to install libpulse on the machines as part of setup
    • This prevents regular developers from having it "just work", that might be acceptable though given it is only for people working on PulseAudio support and is now partitioned out
  • Updating the tests to include a setup step which pulls down and extracts the correct release from https://freedesktop.org/software/pulseaudio/releases/
    • Dependencies may be troublesome to handle
  • Creating nuget packages which contain the appropriate binaries and consuming them "normally"
    • This is the most desirable option but it is also the most complicated and there may be licensing questions/concerns around it.
@FiniteReality

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

I'm thinking the best approach would be to just install libpulse and remove the Windows and Mac OS builds, as PulseAudio is designed primarily for Linux. The support for other platforms seems much like a YMMV situation, and we should likely mirror that.

@tannergooding

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2019

and remove the Windows and Mac OS builds, as PulseAudio is designed primarily for Linux

I'm fine with not running tests on MacOS/Windows and logging a note as to why, but I don't think the builds should be removed. Users should be able to successfully build the product on any .NET Core compatible OS and then copy the resulting files to a a supported target machine and still have them run successfully (provided you don't copy native/external dependencies as well).

@@ -3,6 +3,9 @@ jobs:
pool:
name: ${{parameters.pool}}
steps:
- bash: apt install libpulse-dev

This comment has been minimized.

Copy link
@tannergooding

tannergooding Aug 14, 2019

Collaborator

Does this need to be the dev dependencies and not just the runtime library (given we don't need the header files or static libraries)?

This comment has been minimized.

Copy link
@FiniteReality

FiniteReality Aug 14, 2019

Author Contributor

Personally I've had experiences where installing just the library ('libpulse0' in this case) does not work, still resulting in a DllNotFoundException. In these cases, installing the header files seems to have fixed the issue.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Aug 14, 2019

Collaborator

That's odd. Maybe something to do with the library names not matching or something.

In either case, I'm happy with w/e works 😄

@tannergooding tannergooding merged commit 286f06f into terrafx:master Aug 14, 2019

9 checks passed

terrafx.terrafx.interop.pulseaudio-pr Build #20190814.4 succeeded
Details
terrafx.terrafx.interop.pulseaudio-pr (macos_debug_x64) macos_debug_x64 succeeded
Details
terrafx.terrafx.interop.pulseaudio-pr (macos_release_x64) macos_release_x64 succeeded
Details
terrafx.terrafx.interop.pulseaudio-pr (ubuntu_debug_x64) ubuntu_debug_x64 succeeded
Details
terrafx.terrafx.interop.pulseaudio-pr (ubuntu_release_x64) ubuntu_release_x64 succeeded
Details
terrafx.terrafx.interop.pulseaudio-pr (windows_debug_x64) windows_debug_x64 succeeded
Details
terrafx.terrafx.interop.pulseaudio-pr (windows_debug_x86) windows_debug_x86 succeeded
Details
terrafx.terrafx.interop.pulseaudio-pr (windows_release_x64) windows_release_x64 succeeded
Details
terrafx.terrafx.interop.pulseaudio-pr (windows_release_x86) windows_release_x86 succeeded
Details

@FiniteReality FiniteReality deleted the FiniteReality:initial-bindings branch Aug 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.