[405] fix broken ClangdOptionsDefault

[ghentschke]

fixes #405

[ghentschke]

I am not sure if we have to extend all other public interfaces well (e.g. ClangdMetadataDefaults -> ClangdMetadataDefaults2, ...)

[ghentschke]

That would require every time we expand the interface for new clangd settings entry a lot of new files... :-(

[ruspl-afed]

I have a question: why don't we add new method with default implementation to the existing interface?

[jonahgraham]

I haven't been reviewing or paying particularly close attention to how the preferences are handled, but because @ruspl-afed mentioned new methods in interfaces I want to point out CDT's policy on this https://github.com/eclipse-cdt/cdt/blob/main/POLICY.md#new-default-methods-in-interfaces which is more permissive than API tools allows by default.

PS Along with CDT 12 being a major new version, we could also do a major new version for CDT-LSP too (3.0) if it simplifies our collective lives.

[ghentschke]

I have a question: why don't we add new method with default implementation to the existing interface?

Do you mean to ClangdOptions and increase the version to 3.0.0 because of an API break?

[ghentschke]

I think according to @jonahgraham reply I think we comply with the policy when we add a new default method to the ClangdOptions interface to enbale the console logging like:

	default boolean logToConsole() {
		return false;
	}

[jonahgraham]

I think according to @jonahgraham reply I think we comply with the policy when we add a new default method to the ClangdOptions interface to enbale the console logging like:

Yes, that looks correct. And add a commented filter from the quick fix on the problem that tells you to update major version. Here is an example of what I mean in case you aren't familiar:

This will create a new file .api_filters that should be committed too.

[ruspl-afed]

Thank you @jonahgraham for highlighting this opportunity for us.

@ghentschke do you think we can have less pain in the future if we will declare ClangdOptions2 (or another one with different naming) as independent from ClangdOptions?

Or may be we can even use this opportunity (CDT 12.0) to adapt option management design to the fact that every interface change is in fact the major one, because it seems that we need to avoid any aggregating interfaces.
WDYT @ghentschke @jonahgraham ?

[jonahgraham]

I'm actively not weighing in on @ruspl-afed questions from #406 (comment) because I simply don't have anywhere the experience or use case knowledge to know what the right way forward is. Please let me know if you need me to become more informed on this and weigh in.

[ghentschke]

@ghentschke do you think we can have less pain in the future if we will declare ClangdOptions2 (or another one with different naming) as independent from ClangdOptions?

That was my last approach before we decided to extend ClangdOptions.

Or may be we can even use this opportunity (CDT 12.0) to adapt option management design to the fact that every interface change is in fact the major one, because it seems that we need to avoid any aggregating interfaces.
WDYT @ghentschke @jonahgraham ?

So with every major release the API can be modified?

[ghentschke]

Hm, my IDE does not show API errors. How can I enable them?

[ruspl-afed]

Please let me know if you need me to become more informed on this and weigh in.

I have a hope we can solve this with @ghentschke without bothering the big guys.

[jonahgraham]

Hm, my IDE does not show API errors. How can I enable them?

See https://github.com/eclipse-cdt/cdt/blob/main/POLICY.md#Using-API-Tooling - but it just occurred to me that cdt-baseline.target should have CDT-LSP's latest release and features added to it, or else you won't get errors about CDT-LSP changes. I will do a PR for that in a moment on CDT and tag it here.

[jonahgraham]

See/follow eclipse-cdt/cdt#1066 - I haven't finished testing it is correct, but will do so soon.

[ruspl-afed]

That was my last approach before we decided to extend ClangdOptions.

It would be great if we can have time this week to discuss pros & cons. The current solution with a major version update is workable, but something is wrong if we need to do this for every added option..

So with every major release the API can be modified?

I believe yes

[ruspl-afed]

but it just occurred to me that cdt-baseline.target should have CDT-LSP's latest release and features added to it, or else you won't get errors about CDT-LSP changes.

thank you for the fix @jonahgraham

This explains the need to re-publish existing interface for the new version that we discussed here @ghentschke

[ghentschke]

Finally it got the API filter working. Thank you @jonahgraham !! Any open objections for now, besides the open question how to handle option changes in the future?
Regarding #406 (comment) I'll talk to @ruspl-afed about it.

[jonahgraham]

There needs to be a default to be acceptable by the CDT policy.

(I didn't compile/format this)

Suggested change

	PreferenceMetadata<Boolean> logToConsole();
	default PreferenceMetadata<Boolean> logToConsole() { return false; } ;

[jonahgraham]

LGTM - the build failed and I didn't look at why.

[jonahgraham]

LGTM - the build failed and I didn't look at why.

I had a hunch about the problem, so I decided to have a look:

2025-01-29T21:28:54.0182359Z [ERROR] [API ERROR] File ClangdMetadata.java at line 97: The default method org.eclipse.cdt.lsp.clangd.ClangdMetadata.logToConsole() in an interface has been added (location: /home/runner/work/cdt-lsp/cdt-lsp/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/ClangdMetadata.java)
2025-01-29T21:28:54.0185823Z [ERROR] [API ERROR] File MANIFEST.MF at line 6: The major version should be incremented in version 2.2.0, since API breakage occurred since version 2.0.0 (location: /home/runner/work/cdt-lsp/cdt-lsp/bundles/org.eclipse.cdt.lsp.clangd/META-INF/MANIFEST.MF)
2025-01-29T21:28:54.0189407Z [WARNING] [API WARNING] File ClangdMetadata.java at line 23: The API problem filter for: 'The method org.eclipse.cdt.lsp.clangd.ClangdMetadata.logToConsole() in an interface that is intended to be implemented has been added' is no longer used (location: /home/runner/work/cdt-lsp/cdt-lsp/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/ClangdMetadata.java)

This happens because you changed the signature the problem ID in the api filters needs to be updated.

This is what it looks like in the UI:

What it is saying is the filter you added initially (before it was default) is now unneeded (the warning) + the error about the missing version bump.

I have pushed a fix for the problem and hopefully that will build cleanly.

[ghentschke]

/request-license-review

[github-actions]

/request-license-review

License review requests:

• p2/orbit/p2.eclipse.plugin/com.github.jnr.jffi.native.source/1.3.13.v20240215-1616
• p2/orbit/p2.eclipse.plugin/com.github.jnr.jffi.native/1.3.13.v20240215-1616

After all reviews have concluded, re-run the license-vetting check from the Github Actions web-interface to update its status.

Workflow run (with attached summary files):
https://github.com/eclipse-cdt/cdt-lsp/actions/runs/13046764899

[ghentschke]

/request-license-review

[github-actions]

/request-license-review

License review requests:

• p2/orbit/p2.eclipse.plugin/com.github.jnr.jffi.native.source/1.3.13.v20240215-1616
• p2/orbit/p2.eclipse.plugin/com.github.jnr.jffi.native/1.3.13.v20240215-1616

After all reviews have concluded, re-run the license-vetting check from the Github Actions web-interface to update its status.

Workflow run (with attached summary files):
https://github.com/eclipse-cdt/cdt-lsp/actions/runs/13046842501

[ghentschke]

@jonahgraham Thank you!

[ghentschke]

Hm, I have no idea why the licence check still fails :-/

[jonahgraham]

Hm, I have no idea why the licence check still fails :-/

Possibly due to the significant instability in IT infra at Eclipse this week, see past incidents on https://www.eclipsestatus.io/ . There has been an outage nearly everyday this week.

The HEAD of the main branch passed license check so nothing more to do now.

[ghentschke]

The HEAD of the main branch passed license check so nothing more to do now.

Yes, I've triggered a rebuild several times :-/

[ghentschke]

The HEAD of the main branch passed license check so nothing more to do now.

Yes, I've triggered a rebuild several times :-/

[jonahgraham]

Thanks for doing that. I see you are also suffering some instability with GitHub (dup comments above). I have been having GitHub problems all morning so far too and also had dup comments after GitHub initially tells me save of comment failed.