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

feat: scan kits from a folder #4191

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

std-microblock
Copy link

The following changes are proposed:

  • A button to select a folder to scan kits in the kits selection menu.

The purpose of this change

Make the creation of kits easier.

@gcampbell-msft
Copy link
Collaborator

@std-microblock Thanks for your patience, we hope to assess this as soon as we can, but it may not be until after the holidays. Thank you for understanding.

@gcampbell-msft gcampbell-msft added this to the 1.20 milestone Jan 7, 2025
@gcampbell-msft gcampbell-msft self-assigned this Jan 8, 2025
@sinemakinci1
Copy link
Contributor

@std-microblock thank you so much for your contribution, we greatly appreciate it and your support! As a quick update, we plan to take this PR into our extension soon.

As a side note, we plan to prioritize enhancements to CMake presets support moving forward (see issue: #4117). If there is a reason you prefer kits, we would love to know more in the issue I linked.

Copy link
Collaborator

@gcampbell-msft gcampbell-msft left a comment

Choose a reason for hiding this comment

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

A couple of things that would improve this PR have been commented / requested. Otherwise, I think the changes look reasonable, please ping me when you've updated the PR based on my comments!

@@ -6,6 +6,7 @@ Features:

- Add support for Presets v9, which enables more macro expansion for the `include` field. [#3946](https://github.com/microsoft/vscode-cmake-tools/issues/3946)
- Add support to configure default folder in workspace setting. [#1078](https://github.com/microsoft/vscode-cmake-tools/issues/1078)
- Add the scan kits from a folder option
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please put a link to this PR following the pattern from other entries.

if (!dir || dir.length === 0) {
return false;
}
const dirPathWithDepth = async (folder: string, depth: number = 5) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Confirming my understanding, this is a recursive method adding ALL directories under the selected folder to the paths to search?

Copy link
Author

Choose a reason for hiding this comment

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

Nah, It has a depth limit of 5.

if (!dir || dir.length === 0) {
return false;
}
const dirPathWithDepth = async (folder: string, depth: number = 5) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm noticing that thsi code is duplicated in the kitsController.ts and the presetsController.ts. Please extract it into a method so that we share code. Likely the best place to put it is in the kitsController alongside scanForKits or in kit.ts.

canSelectFiles: false,
canSelectFolders: true,
canSelectMany: false,
openLabel: localize('select.folder', 'Select Folder')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I think it may be a good idea to ensure somewhere that it will also search recursively within the folder. Either here or in the popup that asks to either scan for kits or choose a specific folder.

return localize('unspecified.let.cmake.guess', 'Unspecified (Let CMake guess what compilers and environment to use)');
}
if (kit.name === SpecialKits.ScanSpecificDir) {
return localize('search.for.compilers.in.dir', 'Search for compilers in a specific directory');
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should likely make note here or surface to the user that it will recursively search, if that's what you'd like to happen.

@std-microblock
Copy link
Author

I'll look into the changes when I have spare time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Blocked
Development

Successfully merging this pull request may close these issues.

3 participants