search

free-audio / clap

Audio Plugin API
https://cleveraudio.org/
MIT License
1.78k stars 100 forks source link

Do not include draft headers from clap.h #377

Closed glowcoil closed 8 months ago

glowcoil commented 9 months ago

The contents of the ext/draft/ and factory/draft/ directories do not carry the same stability guarantees as the rest of the CLAP headers. Any of the extension or factory definitions in these directories may be abandoned or replaced with an updated version at any time. Thus, plugin and host developers should be careful when making use of them, and should not ship software to end users under the assumption that a draft extension or factory will continue to be supported indefinitely.

Because of these important stability implications, it should be obvious to host and plugin developers when they are making use of draft functionality. Currently, this is not the case, as developers who include the clap.h "everything" header can make use of all draft extensions and factories without the string "draft" even appearing anywhere in their source code (case in point: clap-helpers includes support for a large number of extensions which are in draft as of CLAP 1.1.10, and the word "draft" only appears in a single place).

As a step towards making the distinction between draft and stabilized interfaces more clearly visible to downstream consumers of the CLAP headers, this PR removes the #includes of any draft headers from clap.h, so that developers making use of draft functionality must make the explicit choice to include the relevant headers.

baconpaul commented 9 months ago

Great change

and of course clap helpers can get a CLAP_Includ_draft compile time option and so forth

abique commented 9 months ago

You need to add another header that performs the draft inclusion then. Maybe clap/drafts.h.

abique commented 9 months ago

You need to document that change in the right place so it is clear that this draft.h header exists and can be used.

I'm against the inclusion of draft.h from clap.h based upon a define.

abique commented 9 months ago

This change, is a breaking change which I'd prefer to avoid.

Maybe it is better to have clap.h which includes the drafts and clap-no-draft.h which doesn't. I don't think one ends up implementing a draft extension by mistake, and even so, it isn't a big deal.

baconpaul commented 9 months ago

Why is this a breaking change? No non draft extensions break do they?

robbert-vdh commented 9 months ago

The motivation behind this change is to discourage (though it can't prevent) draft extensions from 'accidentally' stabilizing again because a version has been around for a while and plugins and DAWs have already started shipping releases using the extension, even though it has not yet been finalized and now cannot be modified anymore without breaking those plugins and hosts. Draft extensions should really be draft extensions, with no stability guarantees, and that can be removed or changed at any point in time.

abique commented 9 months ago

Why is this a breaking change? No non draft extensions break do they?

Because some includes will be gone, so it'll break compilation for products which are using those.

baconpaul commented 9 months ago

But we’ve broken compilation before. Removing the host argument which breaks clap helpers for instance.

What’s our approach on breaking changes?

I really think we dodged a bullet being able to keep these drafts stable as is. 1.2 should fix the problems which led to us almost having a problem

baconpaul commented 9 months ago

Also if we promote the draft to non draft then uses of those won’t break right?

abique commented 9 months ago

The motivation behind this change is to discourage (though it can't prevent) draft extensions from 'accidentally' stabilizing again because a version has been around for a while and plugins and DAWs have already started shipping releases using the extension, even though it has not yet been finalized and now cannot be modified anymore without breaking those plugins and hosts. Draft extensions should really be draft extensions, with no stability guarantees, and that can be removed or changed at any point in time.

There is no accidental stabilization. The latest draft iteration when stabilizing will be copied 1:1 to the stable directory. It will always be the same: a draft will be staged for a while until we consider it stable, and we need to have it working in some host and plugins, be in the hand of some users ideally, to be able to gather feedback, maybe telemetry, etc...

Also I don't want to discourage anyone from trying out drafts, it is useful for us for getting feedback, and it isn't like a trap with poison ready to explode.

glowcoil commented 9 months ago

There is no accidental stabilization.

When an extension is still a draft, but is currently being used in shipping products, and we are avoiding making changes to that extension (such as removing /draft from the end of its name) so as to avoid breaking functionality in those products, that extension has been "accidentally stabilized." And it's not up for debate whether this happens or not; it has happened, and that is the situation we are currently in.

glowcoil commented 9 months ago

This change, is a breaking change which I'd prefer to avoid.

This change only breaks compilation for products using draft extensions, about which CLAP makes no stability promises. It does not break any functionality which has been stabilized.

abique commented 9 months ago

But we’ve broken compilation before. Removing the host argument which breaks clap helpers for instance.

What’s our approach on breaking changes?

I really think we dodged a bullet being able to keep these drafts stable as is. 1.2 should fix the problems which led to us almost having a problem

Yes and you were pretty annoyed, so we should not introduce breaking changes just for the sake of it, and the motivation for this change isn't clear to me and I'm not sure it is the right thing to do. I'm not sure this change fixes the problem it pretend to fix.

Gating the usage of draft extension is maybe better done at runtime via a configuration file, a setting or an environment variable than with an include hack.

I'm quite relaxed when it comes to clap-helper and encouraging people to contribute, but clap itself is more serious to me.

As a summary:

abique commented 9 months ago

There is no accidental stabilization.

When an extension is still a draft, but is currently being used in shipping products, and we are avoiding making changes to that extension (such as removing /draft from the end of its name) so as to avoid breaking functionality in those products, that extension has been "accidentally stabilized." And it's not up for debate whether this happens or not; it has happened, and that is the situation we are currently in.

It wasn't accidental.

abique commented 9 months ago

If you want to make a useful change, add a virtual method in clap helpers: virtual bool enableDrafts() const noexcept;.

glowcoil commented 9 months ago

Gating the usage of draft extension is maybe better done at runtime via a configuration file, a setting or an environment variable than with an include hack.

I think this is a great idea. I am very in favor of plugins and hosts gating any usage of draft extensions behind a config option or environment variable. However, that is not something that can be implemented at the point of the CLAP headers and will have to be implemented by hosts, plugins, plugin frameworks, etc.

this means that for anyone using the clap-helpers they get the draft include anyway.

As @baconpaul mentioned above, clap-helpers should also gate draft functionality behind a CMake flag or similar.

This PR is not intended to be a complete solution to the overall issue, just a step in the right direction.

glowcoil commented 9 months ago

If you want to make a useful change, add a virtual method in clap helpers: virtual bool enableDrafts() const noexcept;.

Happy to do this; I think it's a great idea.

glowcoil commented 9 months ago

Review feedback has been addressed (added a draft.h header and a changelog entry).

baconpaul commented 9 months ago

So here’s what I think we want in clap 1.2

  1. a set of 1.1 draft extensions get promoted to non draft in a way that doesn’t break anything. So they are still in clap.h and have the draft name
  2. a User of 1.2 needs to do something extra to include remaining draft extensions both in clap and clap helpers. This may break people using extensions which are desft in 1.1 and 1.2
  3. The name of still draft in 1.2 extensions removes the word draft. This will break people using draft extensions in 1.1 and 1.2

so question on is: is that what we want?

If so one way to enable it is

  1. move the still in 12 draft exteinsions into clap draft.h or whatever
  2. Include that with a configuration which defacto would be a define
  3. Adjust clap helpers to have a similar configuration which sets the clap and clap helper define
  4. Push 1.2

There are ways other than ifdef and include in clap helpers but since clap itself is c99 the pre processor and a define seem the most idiomatic.

I agree this pr isn’t all of the above, perhaps it should be. But is this where we want to go?

I guess if there are extensions which will still be draft in 1.2 and have production usage, we should understand why.

And as to “accidentally” promoted. Whatever. The point is they were defacto promoted without a version number and a complete signoff. We got lucky and the only problem caused was we end up with a slightly embarrasing draft in a not very user facing string, but we could have got not lucky. I think that’s what we want to avoid going forward,

abique commented 8 months ago

It is still missing the information somewhere that if you want only the stable extensions then you #include <clap/clap.h> and if you want everything #include <clap/all.h>.

Even the names, I'm not sure what would be correct. Including the draft should not produce a negative feeling.

You need to add something into the README.md as well.

abique commented 8 months ago

You probably have to add something to conventions/extension-id.md in the draft section.

abique commented 8 months ago

I think it is a lot of changes for a very hypothetical "fix" for an hypothetical problem.

abique commented 8 months ago

I merge it, but I'm annoyed: this isn't finished both in its form and complete design, it came up late regarding our original plan for 1.2.

wrl commented 8 months ago

This change, is a breaking change which I'd prefer to avoid.

Breaking how? If a project with a CLAP dependency is using a draft extension and bumps its CLAP version, then that project will need to add the relevant includes for the draft extensions. If a project is not using any draft extensions, nothing will break, nothing needs to change.

Also – what's the gain of having a drafts.h include? If a project uses draft extensions, requiring them to be included extension-by-extension serves as an explicit opt-in. Having an overarching drafts.h include lets us continue sweeping the issue under the rug.

I think it is a lot of changes for a very hypothetical "fix" for an hypothetical problem.

Firstly, a large part of stewardship and steering of vendor-neutral standards (and API design more generally) is anticipating and planning for "hypothetical" situations. Furthermore, in this case, the community thinks this is a good idea and does not consider this problem hypothetical.

Secondly, CLAP has only recently dealt with unintentional stabilisation of several draft extensions (preset-discovery and context-menu, might be others I'm blanking on), which again means this is very much not hypothetical.

wrl commented 8 months ago

Also, @abique –

It wasn't accidental.

Who made the decision to stabilise and where was it documented?

abique commented 8 months ago

Also, @abique –

It wasn't accidental.

Who made the decision to stabilise and where was it documented?

Are you trying to piss me off?

You're just wasting my time and energy with BS.

glowcoil commented 8 months ago

Are you trying to piss me off?

You're just wasting my time and energy with BS.

My intentions with this PR and the surrounding discussion have come entirely from a genuine desire to see the CLAP project succeed, and although I won't try to speak on his behalf, I can only assume the same for @wrl.

You may want to reconsider responding to well-intentioned contributions with this kind of hostility and unprofessionalism. It's not something I would personally consider acceptable for the projects that I maintain, and I don't think it will help the CLAP project in the long run.