[VSC-1409] Add check for missing compile_commands.json in IDF projects #1271

radurentea · 2024-08-05T13:33:58Z

Description

This pull request adds a new feature to improve the user experience when working with ESP-IDF projects in VS Code, particularly when using the Microsoft C/C++ extension.

Key changes:

Implements a check for the existence of the compile_commands.json file when opening an ESP-IDF project.
If the file is missing, displays a notification to the user with an option to generate it.

For users using the Microsoft's C/C++ compile_commands.json file is crucial for proper IntelliSense functionality in ESP-IDF projects.

JIRA https://jira.espressif.com:8443/browse/VSC-1409

Type of change

New feature (breaking change which adds functionality)

Steps to test this pull request

Open an ESP-IDF project that doesn't have a compile_commands.json file in its build directory.
Verify that a notification appears, informing about the missing file and offering to generate it.
Click the "Generate compile_commands.json" button and confirm that it triggers the espIdf.idfReconfigureTask command that generates the compile_command.json file in build folder of the project.

We should also test the following scenarios:

fix support for multiple sdkconfig files #1252
Fix compiler args for white spaces #1220
Building should be tested for esp-idf v4.3.6 as well (it was reported in the past by a customer that -S was not accepted, only -S=)

How has this been tested?

As described above

Test Configuration:

ESP-IDF Version: 5.3
OS (Windows,Linux and macOS): Windows 11

Checklist

github-actions · 2024-08-05T15:28:58Z

Download the artifacts for this pull request:
You can test these changes by installing this VSIX by click menu View -> Command Palette..., type Install from VSIX and then select downloaded esp-idf-extension.vsix file to install the extension.

esp-idf-extension.vsix.zip
Espressif documentation preview

brianignacio5

Few changes I consider would improve this PR

src/logger/utils.ts

src/extension.ts

radurentea · 2024-08-06T09:48:17Z

Hi @brianignacio5, thank you for the review!
I've made all the modifications, can you please take another look?

brianignacio5

LGTM

radurentea · 2024-08-14T08:35:36Z

Hi @AndriiFilippov, PTAL

github-actions · 2024-08-30T00:21:57Z

Pull request has been marked as stale since there are no activities, and this will be closed in 5 days if there are no further activities

radurentea · 2024-09-10T08:04:38Z

Hi @Fabricio-ESP, PTAL

github-actions · 2024-09-26T00:23:05Z

Pull request has been marked as stale since there are no activities, and this will be closed in 5 days if there are no further activities

Fabricio-ESP · 2024-10-01T12:49:54Z

The extension is properly detecting the missing file in the builds folder and running the reconfigure command. Tested on Windows and Linux.

Side effect is that when opening a new project (from a example) the build folder does not exist yet, and the error notification will be triggered. Running the reconfigure command will generate the build for the ESP32 default target. If the user wants to change the target the build folder will need to recreated.

If the user has set multiple configuration profiles and the build folder have other names than "build" the extension is showing the error that the compile_commands.json is missing, but they are properly saved on each build folder, so there is something there to detect the file on this scenario.

src/extension.ts

brianignacio5

LGTM

Fabricio-ESP

Tested on Windows, WSL and Linux.
Reconfigure command working as expected.

brianignacio5

Rebase issues.

brianignacio5 · 2024-11-01T08:57:39Z

src/build/buildTask.ts

@@ -129,12 +127,6 @@ export class BuildTask {
          "-DPYTHON_DEPS_CHECKED=1",
          "-DESP_PLATFORM=1",
        ];
-      } else if (useEqualSign) {


The rebase is removing changes from #1266 which are implemented in master.

Please update.

These changes were intentional and are part of the commit Remove unnecessary validation - c8f9ec8

After some testing, I found out the implementation with "=" was unnecessary. After the changes we've made with the configuration settings. These commands are being run in the python env, so the behaviour is the same for all shells the "=" was added for some specific case Andrii found in the past with consecutive white spaces in the name.

I can't see the commit since it is lost maybe due to squash commits.

These commands are being run in the python env ? How ? where?

Hi @brianignacio5,

After spending quite some time on debugging the matter I can give a proper explanation:

I can't see the commit since it is lost maybe due to squash commits.

Unfortunately, yes. But the changes are the same as in this one 00c0620

These commands are being run in the python env ? How ? where?

Disregard this comment, I've made some bad assumptions. Here are my reason for removing the "=" from the commands:

The "=" was introduced in the following PR Fix compiler args for white spaces #1220
I've tested this part of functionality (with the code from this PR) and I was not able to reproduce the error Andrii found with multiple spaces in dev containers anymore. It seems adding the "=" was not the solution for it. Since this was the only reason we've added it in the first place, I think we can safely remove it.

To summary the changes in the PR you've mentioned Fix CMake args for esp-idf lower than 4.4 #1266 related to supporting v4.4:
For version 4.4 I was removing the "=" in the commands, but kept it for any version bigger than that, but again this implementation was not necessary as I've explained in the bullet point above. So removing this implementation won't affect the users with version 4.4 of esp-idf.

As a conclusion, I think adding the "=" was a mistake on my part and there is no reason to use it. I don't fully understand how this implementation solved Andrii's problem, since I've retested it without the "=" and I was not able to reproduce the issue even though I've followed the same steps described by Andrii in his Jira task https://jira.espressif.com:8443/browse/VSC-1381

Create project with whitespaces in name "project-n am e" -> re-open in devcontainer -> try to build -> failed.

I think before merging this task @Fabricio-ESP should test the following scenarios once again just to make sure we are not missing anything:

fix support for multiple sdkconfig files #1252

[VSC-1409] Add check for missing compile_commands.json in IDF projects #1271

Fix compiler args for white spaces #1220

What do you think?

I understand your logic but I think is better in the future to separate this into 2 PRs: 1) compile_commands.json work and 2) The removal of =. When I see the changes I just thought of it as a regression because it changes code unrelated to the PR.

We can proceed testing it in this PR but then is better to add how is fixed in the PR description and steps to test.
I think the issue was an old CMake version (Please test with esp-idf v4.3.6) didn't support -S only -S= and it was reported by a customer, hence my remark to make sure this does work.

Hi @brianignacio5,

I agree with your vision of splitting PRs, but in this particular case there is one thing that I forgot to mention: which is that I've removed the "=" because there was an issue on this PR that Fabricio found and it was caused by the compiler argument "-B=".

Executing task: /home/fabricio/.espressif/python_env/idf5.3_py3.12_env/bin/python /home/fabricio/esp/v5.3.1/esp-idf/tools/idf.py -B=/home/fabricio/Workspace/WSL-Local/blink/build -DSDKCONFIG=/home/fabricio/Workspace/WSL-Local/blink/sdkconfig reconfigure

I will be updating the testing steps in the description.

src/espIdf/reconfigure/task.ts

src/extension.ts

- Removed helper function from logger/utils - Moved code in already existing event listener - Wrapped event listener with context.subscriptions.push() - Localized messages displayed to user

We don't need to use "=" in the commands anymore

There is no situation in which we need "-G=Ninja"

brianignacio5 · 2024-11-11T08:47:52Z

Test building blink on Windows with blink using v5.3.1 and esp-idf 4.3.6 and build works fine.

brianignacio5

LGTM

Fabricio-ESP · 2024-11-15T15:38:01Z

@radurentea
Tested on Windows and Ubuntu, also using docker container and multi configuration.
All working as expected

radurentea requested a review from brianignacio5 August 5, 2024 13:33

radurentea self-assigned this Aug 5, 2024

brianignacio5 requested changes Aug 6, 2024

View reviewed changes

src/logger/utils.ts Outdated Show resolved Hide resolved

src/extension.ts Outdated Show resolved Hide resolved

src/extension.ts Outdated Show resolved Hide resolved

src/extension.ts Outdated Show resolved Hide resolved

radurentea requested a review from brianignacio5 August 6, 2024 09:48

brianignacio5 approved these changes Aug 14, 2024

View reviewed changes

github-actions bot added the stale Stale PR or Issue label Aug 30, 2024

brianignacio5 removed the stale Stale PR or Issue label Aug 30, 2024

github-actions bot added the stale Stale PR or Issue label Sep 26, 2024

brianignacio5 added this to the 1.9.0 milestone Sep 26, 2024

radurentea added ongoing Ongoing Issue or PR, this label will be used for issue or PR which is to be excluded by stale bot and removed stale Stale PR or Issue labels Sep 30, 2024

radurentea force-pushed the enhance/opening-projects-compile-error-solution branch from 5de3252 to f96d071 Compare October 1, 2024 07:45

brianignacio5 requested changes Oct 9, 2024

View reviewed changes

src/extension.ts Show resolved Hide resolved

Fabricio-ESP changed the title ~~Add check for missing compile_commands.json in IDF projects~~ [VSC-1409] Add check for missing compile_commands.json in IDF projects Oct 22, 2024

radurentea requested a review from brianignacio5 October 23, 2024 15:04

radurentea force-pushed the enhance/opening-projects-compile-error-solution branch from a85d68b to c8a4e82 Compare October 25, 2024 07:12

brianignacio5 approved these changes Oct 25, 2024

View reviewed changes

brianignacio5 requested a review from Fabricio-ESP October 25, 2024 08:18

Fabricio-ESP approved these changes Oct 28, 2024

View reviewed changes

radurentea force-pushed the enhance/opening-projects-compile-error-solution branch from bf43756 to 0fa8e6c Compare October 28, 2024 13:23

brianignacio5 requested changes Nov 1, 2024

View reviewed changes

radurentea added 2 commits November 6, 2024 10:54

Add check for missing compile_commands.json in IDF projects

c5251ad

Set of changes for 1st Code Review

0662805

- Removed helper function from logger/utils - Moved code in already existing event listener - Wrapped event listener with context.subscriptions.push() - Localized messages displayed to user

radurentea added 2 commits November 6, 2024 11:45

Use buildPath param

50ed043

Remove unnecessary validation

00c0620

We don't need to use "=" in the commands anymore

radurentea force-pushed the enhance/opening-projects-compile-error-solution branch from 0fa8e6c to 00c0620 Compare November 6, 2024 09:50

radurentea added 2 commits November 6, 2024 12:04

Fix lint

3b55d49

Fix leftovers from unnecessary validation

e04a4fe

There is no situation in which we need "-G=Ninja"

brianignacio5 approved these changes Nov 11, 2024

View reviewed changes

Fabricio-ESP approved these changes Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VSC-1409] Add check for missing compile_commands.json in IDF projects #1271

[VSC-1409] Add check for missing compile_commands.json in IDF projects #1271

radurentea commented Aug 5, 2024 •

edited

Loading

github-actions bot commented Aug 5, 2024 •

edited

Loading

brianignacio5 left a comment

radurentea commented Aug 6, 2024

brianignacio5 left a comment

radurentea commented Aug 14, 2024

github-actions bot commented Aug 30, 2024

radurentea commented Sep 10, 2024

github-actions bot commented Sep 26, 2024

Fabricio-ESP commented Oct 1, 2024 •

edited

Loading

brianignacio5 left a comment

Fabricio-ESP left a comment

brianignacio5 left a comment

brianignacio5 Nov 1, 2024

radurentea Nov 6, 2024

brianignacio5 Nov 6, 2024

radurentea Nov 7, 2024

brianignacio5 Nov 8, 2024

radurentea Nov 8, 2024

brianignacio5 commented Nov 11, 2024

brianignacio5 left a comment

Fabricio-ESP commented Nov 15, 2024

[VSC-1409] Add check for missing compile_commands.json in IDF projects #1271

Are you sure you want to change the base?

[VSC-1409] Add check for missing compile_commands.json in IDF projects #1271

Conversation

radurentea commented Aug 5, 2024 • edited Loading

Description

Type of change

Steps to test this pull request

How has this been tested?

Checklist

github-actions bot commented Aug 5, 2024 • edited Loading

brianignacio5 left a comment

Choose a reason for hiding this comment

radurentea commented Aug 6, 2024

brianignacio5 left a comment

Choose a reason for hiding this comment

radurentea commented Aug 14, 2024

github-actions bot commented Aug 30, 2024

radurentea commented Sep 10, 2024

github-actions bot commented Sep 26, 2024

Fabricio-ESP commented Oct 1, 2024 • edited Loading

brianignacio5 left a comment

Choose a reason for hiding this comment

Fabricio-ESP left a comment

Choose a reason for hiding this comment

brianignacio5 left a comment

Choose a reason for hiding this comment

brianignacio5 Nov 1, 2024

Choose a reason for hiding this comment

radurentea Nov 6, 2024

Choose a reason for hiding this comment

brianignacio5 Nov 6, 2024

Choose a reason for hiding this comment

radurentea Nov 7, 2024

Choose a reason for hiding this comment

brianignacio5 Nov 8, 2024

Choose a reason for hiding this comment

radurentea Nov 8, 2024

Choose a reason for hiding this comment

brianignacio5 commented Nov 11, 2024

brianignacio5 left a comment

Choose a reason for hiding this comment

Fabricio-ESP commented Nov 15, 2024

radurentea commented Aug 5, 2024 •

edited

Loading

github-actions bot commented Aug 5, 2024 •

edited

Loading

Fabricio-ESP commented Oct 1, 2024 •

edited

Loading