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

VS Code Extension Doesn't Recognize Sapling Repo #282

Closed
zengk95 opened this issue Nov 29, 2022 · 10 comments
Closed

VS Code Extension Doesn't Recognize Sapling Repo #282

zengk95 opened this issue Nov 29, 2022 · 10 comments
Assignees
Labels
vscode Sapling VS Code extension

Comments

@zengk95
Copy link

zengk95 commented Nov 29, 2022

When I open up the ISL using VS Code, it gives me on the main screen.

Not a valid repository
c:\Users\kzeng\workspace\crc-worker is not a valid Sapling repository. Clone or init a repository to use ISL.

image

I'm using the latest version Sapling 0.1.20221118-210929-cfbb68aa on windows using Powershell. I can get sl to work in the web browser and on the console as well.

Any recommendations?

@evangrayk
Copy link
Contributor

evangrayk commented Nov 29, 2022

Thanks for reporting this!

If you go to the "output" channel at the bottom, then switch to "Sapling ISL", you'll be able to see the logs from ISL, which should show an error message about why it failed to find a repository.

There's a similar code path here for detecting if the sapling command is not installed correctly. I'm wondering if this is related to "sl" conflicting with Set-Location on powershell.

We don't currently have a VS Code config for the extension to set which sl executable name to use, but if this is related to the powershell issue I think that would be a good first step for us to add.

@evangrayk evangrayk added the vscode Sapling VS Code extension label Nov 29, 2022
facebook-github-bot pushed a commit that referenced this issue Nov 29, 2022
Summary:
When spawning ISL from the CLI, we allow `--command` / infer the command executable from the `sl` in `sl web`.

In vscode, we don't have the same direct place to infer the command from. `sl` is a good guess, and works if you've installed `sl` on your path. But we should allow a config to control exactly what installation / path we spawn ISL from.

This may help with #282

Reviewed By: bolinfest

Differential Revision: D41590875

fbshipit-source-id: 8b909e527e5f0152396ef9263edbeff3eca11f9b
@zengk95
Copy link
Author

zengk95 commented Nov 29, 2022

Hmm you're right. Looks like it's not finding that command. I did rename alias in powershell though

Disposing ISL panel
Populating ISL webview panel
populate isl webview
establish sl client connection for c:\Users\zengk\workspace\test-repo
run command:  sl root
run command:  sl root
run command:  sl config
run command:  sl config
run command:  sl config
Failed to find repository in c:\Users\zengk\workspace\test-repo [Error: Command failed with exit code 1: sl root --noninteractive --config ui.merge=:merge3
'sl' is not recognized as an internal or external command,
operable program or batch file.
	at e.exports (c:\Users\zengk\.vscode\extensions\meta.sapling-scm-0.1.3\dist\extension.js:1:18509)
	at c:\Users\zengk\.vscode\extensions\meta.sapling-scm-0.1.3\dist\extension.js:1:15693
	at process.processTicksAndRejections (node:internal/process/task_queues:96:5)
	at async Fn (c:\Users\zengk\.vscode\extensions\meta.sapling-scm-0.1.3\dist\extension.js:1:85757)
	at async Promise.all (index 0)
	at async kn.getRepoInfo (c:\Users\zengk\.vscode\extensions\meta.sapling-scm-0.1.3\dist\extension.js:1:81984)
	at async c:\Users\zengk\.vscode\extensions\meta.sapling-scm-0.1.3\dist\extension.js:1:87803] {
  shortMessage: 'Command failed with exit code 1: sl root --noninteractive --config ui.merge=:merge3',
  command: 'sl root --noninteractive --config ui.merge=:merge3',
  escapedCommand: 'sl root --noninteractive --config "ui.merge=:merge3"',
  exitCode: 1,
  signal: undefined,
  signalDescription: undefined,
  stdout: '',
  stderr: "'sl' is not recognized as an internal or external command,\r\n" +
    'operable program or batch file.',
  failed: true,
  timedOut: false,
  isCanceled: false,
  killed: false
}
Failed to find repository dotdir in c:\Users\zengk\workspace\test-repo [Error: Command failed with exit code 1: sl root --dotdir --noninteractive --config ui.merge=:merge3
'sl' is not recognized as an internal or external command,
operable program or batch file.
	at e.exports (c:\Users\zengk\.vscode\extensions\meta.sapling-scm-0.1.3\dist\extension.js:1:18509)
	at c:\Users\zengk\.vscode\extensions\meta.sapling-scm-0.1.3\dist\extension.js:1:15693
	at process.processTicksAndRejections (node:internal/process/task_queues:96:5)
	at async Vn (c:\Users\zengk\.vscode\extensions\meta.sapling-scm-0.1.3\dist\extension.js:1:85979)
	at async Promise.all (index 1)
	at async kn.getRepoInfo (c:\Users\zengk\.vscode\extensions\meta.sapling-scm-0.1.3\dist\extension.js:1:81984)
	at async c:\Users\zengk\.vscode\extensions\meta.sapling-scm-0.1.3\dist\extension.js:1:87803] {
  shortMessage: 'Command failed with exit code 1: sl root --dotdir --noninteractive --config ui.merge=:merge3',
  command: 'sl root --dotdir --noninteractive --config ui.merge=:merge3',
  escapedCommand: 'sl root --dotdir --noninteractive --config "ui.merge=:merge3"',
  exitCode: 1,
  signal: undefined,
  signalDescription: undefined,
  stdout: '',
  stderr: "'sl' is not recognized as an internal or external command,\r\n" +
    'operable program or batch file.',
  failed: true,
  timedOut: false,
  isCanceled: false,
  killed: false
}

@evangrayk
Copy link
Contributor

dce1ec6 updates our logic to try sl.exe on windows, and also adds a vscode setting so you could customize this to a specific installation location, hopefully that should help. I'll do a new vscode extension release with this change.

@evangrayk
Copy link
Contributor

I'll also add a change so that we detect this as sl not being found on the PATH rather than as an invalid repo. Looks like our ENOENT check doesn't work on windows.

facebook-github-bot pushed a commit that referenced this issue Nov 29, 2022
Summary:
I thought we could depend on execa to manage returning `ENOENT` for invalid command names, but apprently this doesn't always happen on windows.

By making this test work on windows, we show a more accurate "sl is not installed correctly" rather than "this is an invalid repository".

See this discussion for more information:
sindresorhus/execa#469 (comment)

Example of a user on windows not triggering ENOENT:
#282 (comment)

Instead, we should detect invalid commands on windows with exit code 1 (but only on windows).

The use of this detection is isolated to findRoot, which is where we determine if the command being used is valid. If some other issue triggered this code path, showing "this is not a valid sl command" is also a reasonable error message. Plus we still log the error for debugging purposes.

This may also help with #282.

Reviewed By: bolinfest

Differential Revision: D41595211

fbshipit-source-id: 7cff8eafd58f6da14bb85f4289eb25ddbcf5273c
@evangrayk
Copy link
Contributor

A new version (0.1.5) of the vscode extension has been published with these fixes. Let us know if this helps, or if you still see problems with this!

@zengk95
Copy link
Author

zengk95 commented Dec 1, 2022

It works! The only issue was that the open link for the installation docs didn't work in the error screen (maybe a VS Code issue) and I had to add the sapling directory to my environment variables in windows.

@bolinfest
Copy link
Contributor

The only issue was that the open link for the installation docs didn't work in the error screen (maybe a VS Code issue)

which error screen?

and I had to add the sapling directory to my environment variables in windows

You mean you had to add the folder with sl.exe to your %PATH%?

@zengk95
Copy link
Author

zengk95 commented Dec 5, 2022

This error screen
image

The button didn't work for me.

And yea had to add the folder with sl.exe into the environment variables through the GUI in windows.

@bolinfest
Copy link
Contributor

Ah, so I believe the logic for the button is here:

<VSCodeButton
appearance="secondary"
onClick={e => {
platform.openExternalLink('https://sapling-scm.com/docs/introduction/installation');
e.preventDefault();
e.stopPropagation();
}}>
<T>See installation docs</T>
</VSCodeButton>,

The client portion of openExternalLink() is here:

openExternalLink: url => {
window.clientToServerAPI?.postMessage({type: 'platform/openExternal', url});
},

which should be handled here:

case 'platform/openExternal': {
vscode.env.openExternal(vscode.Uri.parse(message.url));
break;
}

so not sure where things are going wrong.

facebook-github-bot pushed a commit that referenced this issue Dec 9, 2022
…en starting. (#307)

Summary:
This was reported as part of #282.
Specifically, if `sl` (or `sl.exe`) is not found on the `$PATH`,
ISL shows an error box with a **See installation docs** button,
and clicking it sends a `platform/openExternal` message to the server to open an external URL.

This message was not getting processed because the handler
was only processing messages when the `currentState` of
`ServerToClientAPI` was `"repo"` (i.e., not `loading` or `error`).

Failing to find `sl` put things in the `error` state, so the
`platform/openExternal` message was ignored.
Note that the bulk of the messages are processed by
`handleIncomingMessageWithRepo()`, which reads the
`Repository` off `this.currentState`, though that is not
present when in the `loading` or `error` state.

As such, this diff adds a conservative fix, which is to
handle only messages of type `PlatformSpecificClientToServerMessages`,
as `platform.handleMessageFromClient()` can be called
without a `Repository` object.

Pull Request resolved: #307

Test Plan: Verified clicking the **See installation docs** button works now.

Reviewed By: ndmitchell

Differential Revision: D41845324

Pulled By: bolinfest

fbshipit-source-id: 4d90f4dda022c82951f71f22206d424afcfbd4c4
@bolinfest
Copy link
Contributor

I just published v0.1.6 of the VS Code extension, which includes the fix for the "See installation docs" button not working: 08df5af. That turned out to be a subtle issue, so thanks for reporting, @zengk95!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vscode Sapling VS Code extension
Projects
None yet
Development

No branches or pull requests

3 participants