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

Do not include draft headers from clap.h #377

Merged
merged 2 commits into from
Jan 8, 2024

Conversation

micahrj
Copy link
Contributor

@micahrj micahrj commented Jan 7, 2024

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.

@micahrj micahrj changed the title Do not include draft extension headers from clap.h Do not include draft headers from clap.h Jan 7, 2024
@baconpaul
Copy link
Collaborator

Great change

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

Copy link
Member

@robbert-vdh robbert-vdh left a comment

Choose a reason for hiding this comment

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

Good idea. Do you mind adding a changelog entry for this?

@abique
Copy link
Contributor

abique commented Jan 7, 2024

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

@abique
Copy link
Contributor

abique commented Jan 7, 2024

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 abique self-assigned this Jan 7, 2024
@abique abique self-requested a review January 7, 2024 12:34
@abique abique removed their assignment Jan 7, 2024
Copy link
Contributor

@abique abique left a comment

Choose a reason for hiding this comment

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

Please update the PR according to the change Robbert requested and I requested.

@abique
Copy link
Contributor

abique commented Jan 7, 2024

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

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

@robbert-vdh
Copy link
Member

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

abique commented Jan 7, 2024

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

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

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

@abique
Copy link
Contributor

abique commented Jan 7, 2024

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.

@micahrj
Copy link
Contributor Author

micahrj commented Jan 7, 2024

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.

@micahrj
Copy link
Contributor Author

micahrj commented Jan 7, 2024

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

abique commented Jan 7, 2024

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:

  • should host and plugin be aware and take precaution when using drafts?
    • yes, they know they are using draft because it is in the draft folder
  • encourage or discourage usage of draft?
    • encourage it, because we need feedback and we need to use the draft in order to stabilize them
  • clap-helper should provide draft support?
    • ideally yes, if somebody has the time to contribute it => this means that for anyone using the clap-helpers they get the draft include anyway.
    • maybe clap helper can be improved to make it visible that there are some draft extensions implemented, like a method to produce a plugin info summary.

@abique
Copy link
Contributor

abique commented Jan 7, 2024

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 abique marked this pull request as draft January 7, 2024 17:04
@abique
Copy link
Contributor

abique commented Jan 7, 2024

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

@micahrj
Copy link
Contributor Author

micahrj commented Jan 7, 2024

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.

@micahrj
Copy link
Contributor Author

micahrj commented Jan 7, 2024

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.

All includes of draft headers have been moved from clap.h to a separate draft.h
header.
@micahrj
Copy link
Contributor Author

micahrj commented Jan 7, 2024

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

@baconpaul
Copy link
Collaborator

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,

@@ -0,0 +1,12 @@
#pragma once

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing #include "clap.h"

@@ -0,0 +1,12 @@
#pragma once
Copy link
Contributor

@abique abique Jan 8, 2024

Choose a reason for hiding this comment

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

Maybe rename the file to all.h

@abique
Copy link
Contributor

abique commented Jan 8, 2024

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

abique commented Jan 8, 2024

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

@abique
Copy link
Contributor

abique commented Jan 8, 2024

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

@abique abique marked this pull request as ready for review January 8, 2024 11:48
@abique abique merged commit 06d6c85 into free-audio:next Jan 8, 2024
1 of 4 checks passed
@abique
Copy link
Contributor

abique commented Jan 8, 2024

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

wrl commented Jan 8, 2024

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

wrl commented Jan 8, 2024

Also, @abique

It wasn't accidental.

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

@abique
Copy link
Contributor

abique commented Jan 9, 2024

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.

@micahrj
Copy link
Contributor Author

micahrj commented Jan 9, 2024

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.

@free-audio free-audio locked as too heated and limited conversation to collaborators Jan 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants