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 clap::helpers::Host #176

Closed
wants to merge 2 commits into from
Closed

Conversation

Trinitou
Copy link
Contributor

@Trinitou Trinitou commented Oct 16, 2023

This is how I think clap::helpers::Host would be used for clap-wrapper

If considering merging, please make sure that before

@Trinitou Trinitou marked this pull request as draft October 16, 2023 00:38
@Trinitou Trinitou changed the title use clap::helpersHost Use clap::helpersHost Oct 16, 2023
@Trinitou Trinitou changed the title Use clap::helpersHost Use clap::helpers::Host Oct 16, 2023
@Trinitou Trinitou mentioned this pull request Oct 16, 2023
3 tasks
@Trinitou Trinitou marked this pull request as ready for review November 29, 2023 23:43
CPMAddPackage(
NAME "clap-helpers"
GITHUB_REPOSITORY "free-audio/clap-helpers"
GIT_TAG "7b53a685e11465154b4ccba3065224dbcbf8a893"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this a problem, @baconpaul ?
Actually I'd like to use a version tag / branch here (instead of commit hash) but we do not have that for clap-helpers yet

@defiantnerd
Copy link
Collaborator

defiantnerd commented Jan 27, 2024

I am not sure if we should include this. The clap-wrapper solves specific problems by "providing the truth" to remove the impedance mismatch between the different plugin standards, like threading models. So checking for those is kind of unnecessary.

Also, it adds another dependency which is something I wanted to avoid: only the SDKs that the wrapper is being built for should be the only ones needed.

I am more in favor to follow the path for the injector plugin to check CLAP conformity between wrapper and target plugin.

@defiantnerd defiantnerd self-assigned this Jan 27, 2024
@Trinitou
Copy link
Contributor Author

Trinitou commented Jan 27, 2024

I am not sure if we should include this. The clap-wrapper solves specific problems by "providing the truth" to remove the impedance mismatch between the different plugin standards, like threading models. So checking for those is kind of unnecessary.

If we would set CheckingLevel to None, those would be gone basically gone. What would remain then? Just the basic C++ wrapper class around clap_host + all clap_host_s. I think it still would be beneficial even without any checks as the clap-helpers would be the place where new extensions will be added in the future. Then overriding the methods in clap-wrapper will be very fast to do + we can reuse this kind of C-to-C++ wrapping work instead of redoing it for every project again. Also I think it's good to have the clap-wrapper as a kind of standard/example user of clap-helpers who brings in certain requirements/goals for clap-helpers to the table (being zero-overhead at minimum checking level, easy to include, etc.).
So I think both clap-wrapper and clap-helpers could benefit from it. But maybe that's not enough?

Also, it adds another dependency which is something I wanted to avoid: only the SDKs that the wrapper is being built for should be the only ones needed.

Yeah the dependency part is the one I'm also most sceptical about myself. At least @baconpaul has done a great job of leveling-up the clap-helpers CMakeLists.txt so it should now be much more straightforward to have the helpers always in sync with the desired clap-version. (the only missing piece of the puzzle there is the version tag/branch on clap-helpers which already wrote about above)

I am more in favor to follow the path for the injector plugin to check CLAP conformity between wrapper and target plugin.

As already written, I'd be OK with setting CheckingLevel to None. The question is whether the benefits are enough to convince you to add another dependency. Feel free to do whatever you think is the best of course. In the end it's just a proposal.

@Trinitou
Copy link
Contributor Author

Trinitou commented Feb 9, 2024

Now I set the CheckingLevel to None

@Trinitou
Copy link
Contributor Author

Trinitou commented Feb 9, 2024

@defiantnerd please have look into #177 -> there I now condensed the full idea of using clap-helpers in clap-wrapper

@defiantnerd defiantnerd deleted the branch free-audio:next February 11, 2024 11:26
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.

2 participants