-
Notifications
You must be signed in to change notification settings - Fork 217
Fix problem with detecting storage folder on windows #2298
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
Conversation
The `getFirstStoragePath()` method would break on windows: ``` Path contains invalid characters: /c:/git-repo/codespaces-codeql (codeQL.createSkeletonQuery) ``` This makes sense, since we're looking to get the parent folder by splitting for `/`. In windows, paths use `\` instead of `/`. So let's detect the platform and add a test for this case.
14fb1b5 to
3087886
Compare
|
Elena & I paired on testing this on Windows—just double-checking that the fix works in the starter workspace too, and then I'll push up the changes! 🙌🏽 |
We now use `fsPath` instead of `path`. Note: I haven't yet fixed the tests, nor checked manually on mac/linux Tangential change: we now use the `dirname` method, instead of manually splitting paths to get a parent folder.
This should be covered by running the general test suite in CI (on windows-latest)
extensions/ql-vscode/test/vscode-tests/cli-integration/skeleton-query-wizard.test.ts
Outdated
Show resolved
Hide resolved
|
Thanks for fixing this up Shati ❤️ I've just tested this and it downloads the database successfully on Mac as well + creates the QL pack! There's one more thing we need to solve here now that we've made it work on Mac + Windows: we're downloading the database as a workspace folder (see screenshot) when it should actually be using the
I think what we want to do is use two separate paths:
Not sure how much more time you want to spend on this. I'm happy to add an extra commit to separate the two paths but if you want to take the reins, let me know. |
e66ad99 to
58c808f
Compare
|
I've added two extra commits to split out:
I've tested this on Mac for codespaces-codeql & the starter workspace. |
| private readonly databaseUI: DatabaseUI, | ||
| private readonly localQueryResultsView: ResultsView, | ||
| private readonly queryStorageDir: string, | ||
| private readonly ctx: ExtensionContext, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use App here to avoid introducing new dependencies on the ExtensionContext? That should also contain a workspaceStoragePath and globalStoragePath as strings which shortens the code below as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool! Have pushed a change to use App.
Now that we've figured out how to set the storage path for both Mac & Windows, we also want to make sure we're consistent with the location where we save databases. At the moment, our change will download databases directly in the workspace folder. When we call `downloadGitHubDatabase()` in other places outside the wizard, we provide `ctx.storageUri.fsPath` as the location. [1] [2] [3] Let's do the same here. I've tested this on Mac for the codespaces-codeql & starter workspaces. [1]: https://github.com/github/vscode-codeql/blob/c7bb22c312d4bbe46d90ff0a43e9cff22c6bd5ed/extensions/ql-vscode/src/local-databases-ui.ts#L476 [2]: https://github.com/github/vscode-codeql/blob/c7bb22c312d4bbe46d90ff0a43e9cff22c6bd5ed/extensions/ql-vscode/src/extension.ts#L710 [3]: https://github.com/github/vscode-codeql/blob/c7bb22c312d4bbe46d90ff0a43e9cff22c6bd5ed/extensions/ql-vscode/src/extension.ts#L1120
58c808f to
e298f2b
Compare

Discovered by @shati-patel here.
The
getFirstStoragePath()method would break on windows:This makes sense, since we're looking to get the parent folder by splitting for
/. In Windows, paths use\instead of/. So let's detect the platform and add a test for this case.