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)

Builder. Created a new extension point to provide the opportunity to incorporate additional information into the file

generation on false by default. Added JUnit test to test the generation of the file

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)

This was completed so I could best investigate eclipse-cdt#729 (comment)

…#731) Pulled out of eclipse-cdt#729 due to instability in tests caused by a platform change.

Kummallinen self-assigned this Feb 5, 2024

Kummallinen 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

Open

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

alicetrifu added 2 commits March 7, 2024 14:33

Added feature to generate compile_commands.json file, in the Common

8f427bb

Builder. Created a new extension point to provide the opportunity to incorporate additional information into the file

Created JSON Compilation Database Generator preference page and set file

852b0a8

generation on false by default. Added JUnit test to test the generation of the file

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

Fixed selection issue on property page

d530365

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

jonahgraham and others added 4 commits March 21, 2024 16:05

Refactor MarkOccurrenceTest to JUnit5 style (eclipse-cdt#730)

35e9cdd

This was completed so I could best investigate eclipse-cdt#729 (comment)

Add GNU assembler debug level option to MBS

44425fb

Update to Eclipse Platform 4.32 for CDT 11.6 development (eclipse-cdt…

eacd9eb

…#731) Pulled out of eclipse-cdt#729 due to instability in tests caused by a platform change.

Created JSON Compilation Database Generator preference page and set file

245c225

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

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

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

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

Added feature to generate compile_commands.json file #692

Are you sure you want to change the base?

Added feature to generate compile_commands.json file #692

Conversation

alicetrifu commented Feb 5, 2024 • edited by jld01

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

alicetrifu commented Feb 5, 2024 •

edited by jld01