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.

Sign up for GitHub

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

Jump to bottom

Add ignoredWorkspaceFolders setting #3617

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add ignoredWorkspaceFolders setting #3617

wants to merge 2 commits into from
+26 −1

Conversation

gsingh93
Copy link
Contributor

@gsingh93 gsingh93 commented May 20, 2024 •
edited
Loading

This PR adds an ignoredFolders setting to the extension. Currently, getOnDiskWorkspaceFolders gets all root folders in a multi-root workspace to use for things like --additional-packs when invoking the CLI tool. This can cause major latency (multiple minutes) when the workspace folders are large. By ignoring these folders, the extension is significantly faster.

Another way to get the speed up is to add an empty codeql-workspace.yml in the root of the large workspace folder to prevent them from being searched, but I don't want to have to create this file every time I need to do this, as usually I can't add it to upstream projects.

I'll update the CHANGELOG once I get some confirmation this is OK to add.

image

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@gsingh93 gsingh93 requested a review from a team as a code owner May 20, 2024 21:59
robertbrignull
robertbrignull reviewed May 21, 2024
View reviewed changes
Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

This applies to more than just extension packs and would affect everywhere that the extension tries to discover files in the workspace. So it's quite a big change, but not necessarily a bad once and I'm not opposed to it. It could be quite useful for certain cases when you have very large workspace folders that you want to ignore.

extensions/ql-vscode/package.json Outdated
@@ -198,6 +198,11 @@
"maximum": 1024,
"description": "Number of threads for running queries."
},
"codeQL.runningQueries.ignoredFolders": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we rename to codeQL.runningQueries.ignoredWorkspaceFolders. It's a bit longer but it makes it clear just from the name that it only applies to workspace folders and not to arbitrary folders.

@gsingh93 gsingh93 changed the title Add ignoredFolders setting Add ignoredWorkspaceFolders setting May 21, 2024
@gsingh93
Copy link
Contributor Author

Thanks @robertbrignull, I updated with the suggested name and included a line in the changelog. Let me know if you have any other feedback.

Regarding this line in the "Checklist", I'm not sure what needs to be done:

Issues have been created for any UI or other user-facing changes made by this pull request.

@robertbrignull
Copy link
Contributor

Thanks for adding a changelog entry.

Regarding this line in the "Checklist", I'm not sure what needs to be done:

Issues have been created for any UI or other user-facing changes made by this pull request.

Don't worry about that. You can ignore it, and we will tidy up the PR template.

@gsingh93
Copy link
Contributor Author

Ping @robertbrignull. Anything else you need from me?

@robertbrignull
Copy link
Contributor

I'm very sorry for the delay. We have now discussed this internally. Unfortunately we're not sure we want to go this route because the implementation looks more complicated than initially thought.

We don't always use getOnDiskWorkspaceFoldersObjects when dealing with workspace folders. It's a good helper method but there are a large number of places where we access workspace.workspaceFolders directly, including modifying it, and often needing the exact index of a certain element.

As well as the implementation cost it's also not fully clear if this config option is what we will want long term. Is it the right level of granularity or should it apply to things other than workspace folders? Unfortunately it would be hard to change later without breaking user's workspaces.

Overall we might have to ask you to keep using your workaround for now. If this continues to be a problem or if there are other users experiencing the same issue, we can revisit this.

@gsingh93
Copy link
Contributor Author

Thanks for the response. I think your concerns are reasonable. That being said, I'm wondering if there's any path forward for this by just focusing on codeql resolve library-path, which is the main source of the performance issues. Essentially, instead of modifying the global getOnDiskWorkspaceFoldersObjects, we just prevent certain folders from being added to --additional-packs when this resolve command is about to be executed. By scoping it down like this, we address the main issue without affecting other unrelated parts of the extension, and we make the setting name and description clear about exactly it applies to. If in the future we want to expand this to other parts of the extension, at least it wouldn't be a breaking change.

Overall we might have to ask you to keep using your workaround for now.

Funnily enough, the workaround started causing issues this morning. I think after I updated to 2.17.4, codeql complains when it sees an empty workspace file ([ERROR] resolve library-path> ERROR: File is empty.). I fixed it by changing the empty workspace file to just provides:, but this is an example of why relying on unofficially supported workarounds like this long-term is not a great idea. I also need to go make sure my colleagues are aware of this change now, as they don't really know any of the internals of how workspaces work or library paths are resolved, and would likely spend more time debugging this on their own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertbrignull robertbrignull robertbrignull left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@gsingh93 @robertbrignull