Added feature to generate compile_commands.json file #692

alicetrifu · 2024-02-05T16:04:13Z

Added a new generator class to provide a compile_commands.json file in the "build" folder. Created a new extension point to provide the opportunity to incorporate additional information into the file.

Part of #689

Kummallinen

Quick initial pass

You also need to get your API baseline set correctly, as there mare API changes here so you need feedback from the API tooling to get the versions set correctly & all appropriate since tags in place

build/org.eclipse.cdt.managedbuilder.core/META-INF/MANIFEST.MF

.../src/org/eclipse/cdt/managedbuilder/core/jsoncdb/generator/CompilationDatabaseGenerator.java

Kummallinen · 2024-02-05T16:23:06Z

See https://github.com/eclipse-cdt/cdt/blob/main/CONTRIBUTING.md#setup-cdt-for-development-manual-setup for API tooling setup guidance

...clipse/cdt/managedbuilder/core/jsoncdb/generator/CompilationDatabaseContributionManager.java

...rg/eclipse/cdt/managedbuilder/core/jsoncdb/generator/api/CompilationDatabaseInformation.java

...g/eclipse/cdt/managedbuilder/core/jsoncdb/generator/api/ICompilationDatabaseContributor.java

....cdt.managedbuilder.core/src/org/eclipse/cdt/managedbuilder/internal/core/CommonBuilder.java

jonahgraham

I have marked as Resolved those previous comments that seemed complete

build/org.eclipse.cdt.managedbuilder.core/META-INF/MANIFEST.MF

...der.core/src/org/eclipse/cdt/managedbuilder/core/jsoncdb/CompilationDatabaseInformation.java

...clipse/cdt/managedbuilder/core/jsoncdb/generator/CompilationDatabaseContributionManager.java

....cdt.managedbuilder.core/src/org/eclipse/cdt/managedbuilder/internal/core/CommonBuilder.java

...clipse/cdt/managedbuilder/core/jsoncdb/generator/CompilationDatabaseContributionManager.java

.../src/org/eclipse/cdt/managedbuilder/core/jsoncdb/generator/CompilationDatabaseGenerator.java

jonahgraham

The code cleanliness failed, but not because of this PR. Once I merge #699 you will need to rebase this change.

jonahgraham

I have done a partial review - there are still some concerns I have on the generator that I need to look at a little closer.

build/org.eclipse.cdt.managedbuilder.core/META-INF/MANIFEST.MF

...clipse/cdt/managedbuilder/core/jsoncdb/generator/CompilationDatabaseContributionManager.java

jonahgraham · 2024-02-09T20:24:13Z

...clipse/cdt/managedbuilder/core/jsoncdb/generator/CompilationDatabaseContributionManager.java

+		factoryExtensions = new HashMap<>();
+		loadedInstances = new HashMap<>();


I marked previous comments resolved around these fields because they weren't appearing on the latest patchset.

These fields lifecycle is unclear, made more complicated by the local variable with a similar name in initialise method.

Can these fields be final, assigned at declaration and have a clearer lifecycle? If my comment is unclear, let me know and I will provide a commit to explain it.

I hope this is what you've meant, Jonah. If not, please let me know.

This is part of what I meant - I will add a commit to this PR to explain explicitly what I mean.

I added b6c8e30 which shows what I meant. It is fairly minor, but removes some code and gets rid of the extra temporary map.

.../src/org/eclipse/cdt/managedbuilder/core/jsoncdb/generator/CompilationDatabaseGenerator.java

jonahgraham

Not sure at what step we need to add some tests for this. In the meantime, this change (probably because we generate into "new" directory build) causes a test failure in org.eclipse.cdt.managedbuilder.core.regressions.Bug_303953Test.testBuildAfterSourcefileDelete() Please see this section of the testing document for how to run these tests in the dev environment.

jonahgraham · 2024-02-15T16:10:21Z

Following our call yesterday we decided that a bit of work is needed on enabling the feature. Quoted from minutes:

One aspect where a decision agreed (with only minor reservations) by all in attendance is to enable the generator by a workspace preference that is disabled by default, with an API to allow CDT-LSP and other dependent projects (such as Renesas' internal editor) to enable it.

jonahgraham · 2024-02-22T01:25:24Z

Following our call yesterday we decided that a bit of work is needed on enabling the feature. Quoted from minutes:

One aspect where a decision agreed (with only minor reservations) by all in attendance is to enable the generator by a workspace preference that is disabled by default, with an API to allow CDT-LSP and other dependent projects (such as Renesas' internal editor) to enable it.

This has grown in scope - in light of our call today and the comment that @15knots made: #689 (comment) we do indeed need this to be settable with the default being the workspace setting. The workspace setting default should be off for now.

jonahgraham · 2024-02-22T01:26:09Z

We are going to hold off merging this until after we branch. We plan to branch very soon, a little earlier than normal.

jonahgraham · 2024-02-22T01:29:02Z

The conclusion of the meeting today is that the current location for generation (in CommonBuilder) is a reasonable short term solution. There may be better places in the future to keep the compile_commands.json more live, but that is deferred and won't hold up the merging of this change.

Similarly the placement of the output in build/compile_commands.json is a reasonable place for now, but during the meeting we made note that improving the flexibility in clangd may be best here. See also #689 (comment)

Kummallinen · 2024-03-01T14:08:38Z

...clipse/cdt/managedbuilder/core/jsoncdb/generator/CompilationDatabaseContributionManager.java

+ *
+ * SPDX-License-Identifier: EPL-2.0
+ ********************************************************************************/
+package org.eclipse.cdt.managedbuilder.core.jsoncdb.generator;


If this isn't being exported probably should name this package as an internal. so "org.eclipse.cdt.managedbuilder.internal.core.jsoncdb.generator"

alicetrifu · 2024-03-05T11:41:37Z

Hello @jonahgraham ,
I've developed a small test suite for functionality validation. I aimed to verify that the functionality applies correctly to CPP projects as well, and to achieve this, I added a new project to the regression workspace and used that in my test. Unfortunately, this test is causing an error during the test execution process along with the build procedure, here on GitHub.

On isCPPFileAllowed (org.eclipse.cdt.managedbuilder.core.tests.CompilationDatabaseGenerationTest) with error
test-results/build/org.eclipse.cdt.managedbuilder.core.tests/target/surefire-reports/TEST-org.eclipse.cdt.managedbuilder.core.tests.CompilationDatabaseGenerationTest.xml [t
I see the following error:
Contains: Errors running builder 'CDT Builder' on project 'helloworldCPP'.
java.lang.NullPointerException: Cannot invoke "org.eclipse.cdt.managedbuilder.core.IOutputType.getOutputExtensions(org.eclipse.cdt.managedbuilder.core.ITool)" because "outType" is null

It's worth mentioning that I never encountered this exception in the workspace, the output is always different from null. Do you have any suggestions on how I could address this issue, maybe? Currently, I've marked the test as skipped, but I believe this should be investigated further in the future.

jonahgraham

This looks very close to me. I am about to merge in 11.6 version bump now that I have branched for 11.5. I'll try rebasing and see if I can reproduce the difference.

jonahgraham · 2024-03-07T16:10:22Z

...tests/tests/org/eclipse/cdt/managedbuilder/core/tests/CompilationDatabaseGenerationTest.java

+	 * @throws CoreException
+	 */
+	@Test
+	@Ignore("This will be temporary skipped due to builder error")


The difference between what you see in the workspace and what happens on the build machine is probably down to different sets of enabled bundles. See https://github.com/eclipse-cdt/cdt/blob/main/TESTING.md#using-limited-sets-of-plug-ins-in-tests (point 3)

jonahgraham

The preferences page does not work. The performApply calling itself makes it fail catastrophically, but my attempt at the simple fix did not resolve the issue.

I made comments in the PR for most of the changes I made. I'll mark them as resolved, but wanted you to know why I changed what I did.

...tests/tests/org/eclipse/cdt/managedbuilder/core/tests/CompilationDatabaseGenerationTest.java

jonahgraham · 2024-03-07T20:53:08Z

...se/cdt/managedbuilder/internal/ui/compilationdatabase/CompilationDatabaseGeneratorBlock.java

+
+	@Override
+	public void performApply(IProgressMonitor monitor) throws CoreException {
+		this.performApply(monitor);


Not sure what intention was here, but this calls itself and causes stack overflow. (Probably should do what is in the listener on line 71)

I made this change, but the new code needs to be checked that it is correct.

jonahgraham · 2024-03-07T20:55:52Z

...se/cdt/managedbuilder/internal/ui/compilationdatabase/CompilationDatabaseGeneratorBlock.java

+		generateFileCheckbox.setText(Messages.JsonCdbGeneratorPreferencePage_generateCompilationdatabase);
+		generateFileCheckbox.addListener(SWT.Selection, e -> {
+			boolean newValue = generateFileCheckbox.getSelection();
+			preferenceStore.setValue(ENABLE_FILE_GENERATION, newValue);


Lets avoid changing the preference until Apply is pressed

I made this change, but the new code needs to be checked that it is correct.

...se/cdt/managedbuilder/internal/ui/compilationdatabase/CompilationDatabaseGeneratorBlock.java

jonahgraham · 2024-03-10T16:36:26Z

Seeing as this implementation the preferences don't work, some additional time is needed here.

I spent some time trying to replicate how @ghentschke and @ruspl-afed implemented preferences for cdt-lsp (e.g. EditorOptions and related classes). That implementation is a little complicated, but does allow all the configuration by products needed. (Unfortunately I ran out of time to do this before I went on vacation)

Includes: - Created a new extension point to provide the opportunity to incorporate additional information into the file - New tests for the generator - Preference page to enable/disable generator Fixes eclipse-cdt#689

Review is now out of date.

jonahgraham · 2024-05-15T20:05:50Z

@ghentschke @Kummallinen @alicetrifu

I added this additional commit (and squashed the rest)

Generate compile_commands.json into standard build directory. That it is the file ends up in
Debug/compile_commands.json instead of build/compile_commands.json. This lines up the behaviour of the generator with that of the code that generates a .clangd file in ClangdConfigurationFileManager.java This code already supports vendors overriding to supply their own (or extended) .clangd contents.

jonahgraham · 2024-05-15T20:06:15Z

Assuming no objections to #692 (comment) and a clean build, this is ready to merge.

That it is the file ends up in Debug/compile_commands.json instead of build/compile_commands.json. This lines up the behaviour of the generator with that of the code that generates a .clangd file in [ClangdConfigurationFileManager.java](https://github.com/eclipse-cdt/cdt-lsp/blob/ee297ee0ced1747352039e6ee25bcc80c10c7462/bundles/org.eclipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/clangd/internal/config/ClangdConfigurationFileManager.java) This code already supports vendors overriding to supply their own (or extended) .clangd contents.

jonahgraham · 2024-05-16T16:05:53Z

Assuming no objections to #692 (comment) and a clean build, this is ready to merge.

I discussed this with William this morning and he/Renesas are happy to take advantage of the API that @ghentschke has provided in ClangdConfigurationFileManager to handle updating .clangd if and as needed.

Therefore I am merging this now and we can start creating new issues as needed too.

Kummallinen self-assigned this Feb 5, 2024

Kummallinen previously requested changes Feb 5, 2024

View reviewed changes

ruspl-afed reviewed Feb 5, 2024

View reviewed changes

jld01 reviewed Feb 6, 2024

View reviewed changes

....cdt.managedbuilder.core/src/org/eclipse/cdt/managedbuilder/internal/core/CommonBuilder.java Outdated Show resolved Hide resolved

alicetrifu force-pushed the compilationDatabaseGenerator branch from e668dea to e20eb76 Compare February 6, 2024 11:58

jonahgraham requested changes Feb 6, 2024

View reviewed changes

alicetrifu force-pushed the compilationDatabaseGenerator branch from e20eb76 to e1c8915 Compare February 8, 2024 11:33

alicetrifu mentioned this pull request Feb 8, 2024

Add compile-commands.json generation for Managed Build Projects #689

Closed

ruspl-afed reviewed Feb 8, 2024

View reviewed changes

jonahgraham reviewed Feb 8, 2024

View reviewed changes

alicetrifu force-pushed the compilationDatabaseGenerator branch from e1c8915 to beb9200 Compare February 9, 2024 09:40

jonahgraham reviewed Feb 9, 2024

View reviewed changes

alicetrifu force-pushed the compilationDatabaseGenerator branch from beb9200 to e6fb475 Compare February 12, 2024 10:43

jonahgraham added a commit to alicetrifu/cdt that referenced this pull request Feb 12, 2024

Code for eclipse-cdt#692 (comment)

b6c8e30

jonahgraham requested changes Feb 12, 2024

View reviewed changes

alicetrifu force-pushed the compilationDatabaseGenerator branch 2 times, most recently from 743ca47 to a3198c3 Compare February 21, 2024 14:04

alicetrifu force-pushed the compilationDatabaseGenerator branch from a3198c3 to c825859 Compare February 28, 2024 14:42

Kummallinen reviewed Mar 1, 2024

View reviewed changes

alicetrifu force-pushed the compilationDatabaseGenerator branch 2 times, most recently from f7eb2c8 to 224f231 Compare March 5, 2024 11:01

jonahgraham reviewed Mar 7, 2024

View reviewed changes

jonahgraham force-pushed the compilationDatabaseGenerator branch from 224f231 to b9f56ca Compare March 10, 2024 16:28

jonahgraham reviewed Mar 10, 2024

View reviewed changes

jonahgraham force-pushed the compilationDatabaseGenerator branch 4 times, most recently from e58842f to 36aceae Compare March 11, 2024 17:38

alicetrifu force-pushed the compilationDatabaseGenerator branch from 36aceae to d530365 Compare March 15, 2024 08:03

alicetrifu force-pushed the compilationDatabaseGenerator branch from 4cfbe34 to 245c225 Compare April 10, 2024 13:20

jonahgraham force-pushed the compilationDatabaseGenerator branch 2 times, most recently from 9281017 to 502ca15 Compare May 15, 2024 18:23

Added feature to generate compile_commands.json file

1be4ce0

Includes: - Created a new extension point to provide the opportunity to incorporate additional information into the file - New tests for the generator - Preference page to enable/disable generator Fixes eclipse-cdt#689

jonahgraham force-pushed the compilationDatabaseGenerator branch from 0303c3a to deec4b6 Compare May 15, 2024 19:59

jonahgraham linked an issue May 15, 2024 that may be closed by this pull request

Add compile-commands.json generation for Managed Build Projects #689

Closed

jonahgraham marked this pull request as ready for review May 15, 2024 20:00

jonahgraham approved these changes May 15, 2024

View reviewed changes

jonahgraham force-pushed the compilationDatabaseGenerator branch from deec4b6 to bfce795 Compare May 15, 2024 20:05

jonahgraham force-pushed the compilationDatabaseGenerator branch from bfce795 to f13b0cb Compare May 15, 2024 20:52

jonahgraham merged commit 5ca5107 into eclipse-cdt:main May 16, 2024
5 checks passed

jonahgraham added this to the 11.6.0 milestone May 16, 2024

jonahgraham added the noteworthy Pull requests and fixed issues that should be highlighted to users label May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added feature to generate compile_commands.json file #692

Added feature to generate compile_commands.json file #692

alicetrifu commented Feb 5, 2024 •

edited by jld01

Loading

Kummallinen left a comment

Kummallinen commented Feb 5, 2024

jonahgraham left a comment

jonahgraham left a comment

jonahgraham left a comment

jonahgraham Feb 9, 2024

alicetrifu Feb 12, 2024

jonahgraham Feb 12, 2024

jonahgraham Feb 12, 2024

jonahgraham left a comment

jonahgraham commented Feb 15, 2024

jonahgraham commented Feb 22, 2024

jonahgraham commented Feb 22, 2024

jonahgraham commented Feb 22, 2024

Kummallinen Mar 1, 2024

alicetrifu commented Mar 5, 2024

jonahgraham left a comment

jonahgraham Mar 7, 2024

jonahgraham left a comment

jonahgraham Mar 7, 2024

jonahgraham Mar 10, 2024

jonahgraham Mar 7, 2024

jonahgraham Mar 10, 2024

jonahgraham commented Mar 10, 2024

jonahgraham commented May 15, 2024

jonahgraham commented May 15, 2024

jonahgraham commented May 16, 2024

		factoryExtensions = new HashMap<>();
		loadedInstances = new HashMap<>();

Added feature to generate compile_commands.json file #692

Added feature to generate compile_commands.json file #692

Conversation

alicetrifu commented Feb 5, 2024 • edited by jld01 Loading

Kummallinen left a comment

Choose a reason for hiding this comment

Kummallinen commented Feb 5, 2024

jonahgraham left a comment

Choose a reason for hiding this comment

jonahgraham left a comment

Choose a reason for hiding this comment

jonahgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgraham left a comment

Choose a reason for hiding this comment

jonahgraham commented Feb 15, 2024

jonahgraham commented Feb 22, 2024

jonahgraham commented Feb 22, 2024

jonahgraham commented Feb 22, 2024

Choose a reason for hiding this comment

alicetrifu commented Mar 5, 2024

jonahgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgraham commented Mar 10, 2024

jonahgraham commented May 15, 2024

jonahgraham commented May 15, 2024

jonahgraham commented May 16, 2024

alicetrifu commented Feb 5, 2024 •

edited by jld01

Loading