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

Initial multi-root support #121

Merged
merged 7 commits into from
May 4, 2023

Conversation

Discookie
Copy link
Collaborator

@Discookie Discookie commented Mar 9, 2023

Requires #123. Reference: #113, Ericsson/codechecker#3708

Adds support for CodeChecker 6.22's automatic compilation database detection. If no comp.db. is specified, and there is none in CodeChecker's output folder, the extension will use CodeChecker's autodetection (for 6.22 and above). For 6.21, and the mentioned cases, the behavior remains the same.

Remaining tasks:

  • Design and implement commands for building multi-root projects. Currently you can only build either a single file, or the outermost folder via the default commands.
  • Support for anayzing 2 or more files without an explicit comp.db. From my reading, CodeChecker analyze can only accept a single file or folder as input, so for multiple files we'd need to autodetect common folders, or run the analysis for each file separately.
  • Handle failure of CodeChecker analyze if it does not detect a compilation database. Currently it returns code 0, so we'd need to read its output to detect the failure.

@Discookie Discookie added the enhancement New feature or request label Mar 9, 2023
@Discookie Discookie requested a review from vodorok March 9, 2023 11:34
@Discookie Discookie force-pushed the ericsson/multi-workspace branch from 025fdbe to c01b66e Compare March 16, 2023 11:35
The file events apparently did not fire consistently fast enough on CI.
@Discookie Discookie force-pushed the ericsson/multi-workspace branch from c01b66e to 4dcb037 Compare March 28, 2023 11:18
@Discookie Discookie force-pushed the ericsson/multi-workspace branch from 4dcb037 to b5d224b Compare March 28, 2023 11:46
@Discookie
Copy link
Collaborator Author

Rebased, should be ready for review now. Requires #125.

@vodorok vodorok requested a review from bruntib April 4, 2023 13:33
Copy link
Collaborator

@vodorok vodorok left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -84,7 +84,7 @@ Since CodeChecker-related paths vary greatly between systems, the following sett
| Name | Description |
| --- | --- |
| CodeChecker > Backend > Output folder <br> (default: `${workspaceFolder}/.codechecker`) | The output folder where the CodeChecker analysis files are stored. |
| CodeChecker > Backend > Compilation database path <br> (default: *(empty)*) | Path to a custom compilation database, in case of a custom build system. The database setup dialog sets the path for the current workspace only. |
| CodeChecker > Backend > Compilation database path <br> (default: *(empty)*) | Path to a custom compilation database, in case of a custom build system. The database setup dialog sets the path for the current workspace only. Leave blank to use the database in CodeChecker's output folder, or to use CodeChecker's autodetection for multi-root workspaces. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not entirely clear which compilation database will be used when the field is empty.

@@ -57,7 +57,7 @@ export class NotificationHandler {
break;
}

const choiceCommand = options.choices!.find((command) => command.title === choice);
const choiceCommand = options.choices?.find((command) => command.title === choice);
Copy link
Collaborator

Choose a reason for hiding this comment

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

From non-null to optional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Closing the popup via the X returns choice 'undefined', apparently.

@vodorok vodorok merged commit 5a00fa4 into Ericsson:main May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants