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

Dependency checks and environment setup assistance #55

Merged
merged 68 commits into from
Jul 22, 2022

Conversation

petervdonovan
Copy link
Contributor

@petervdonovan petervdonovan commented May 24, 2022

This branch adds checks for various dependencies, with corresponding messages and suggested install commands in the UI. The new behavior is described mostly declaratively in check_dependencies.ts.

This feature has been exceedingly difficult to test. Tests exist, but coverage is incomplete. This is not good, but I think it is acceptable because when the extension sends text to the VS Code terminal, it does not append a terminating newline. This means the user can review the suggested install/update command before pressing Enter if CI is supplemented with manual tests.

These changes are untested.
It was necessary to switch from Jest to Mocha + Chai because Jest does not expose a stable API for programmatic invocation. Programmatic invocation is necessary in order for the tests to have access to the 'vscode' module, a process that involves actually running the tests within a VS Code instance. There is a backdoor way to invoke Jest programmatically, but I failed to find a way to do that while preserving access to the 'vscode' module.
@petervdonovan petervdonovan force-pushed the check-versions branch 24 times, most recently from 080e397 to f65d74e Compare May 25, 2022 00:54
@lhstrh
Copy link
Member

lhstrh commented Jun 17, 2022

Currently, the user is presented with a notification that will typically have a button labeled either "View download page", "Update", or "Install". If they click a button that says "Update" or "Install", an update/install command is sent to the VS Code integrated terminal, which pops up on the user's screen. The problem is that the update/install command might not be what the user wants -- for example, maybe the user wants to manage their packages using brew, but this command bypasses that to do the installation using a bash script. Therefore, I decided to make the user press the Enter key to execute the command in the integrated terminal. Alternatives include:

* Providing more detail in the dialog/button so that the user knows in advance what command it will execute, then executing it in the terminal without waiting for the user to press Enter

* Showing a "[quick pick](https://github.com/microsoft/vscode-extension-samples/tree/main/quickinput-sample)" (sort of like a drop-down menu) that lists all the possible install commands, and lets the user choose one of them. This would have to happen in addition to showing a message explaining why the dependency is needed, because quick picks do not come with explanatory messages.

I think that either of these alternatives are better than requiring the user to press Enter. I would go for the option that is easiest to implement. The key is that a user must be 1) given sufficient information to do a manual install and 2) given the opportunity to back out of an automatic install.

Another issue is what happens when the user performs an action that multiple dependencies depend on. It could be problematic for two messages to pop up at once, because the user may take several seconds to handle the first message, during which time the second message may disappear. (It takes 20 seconds for a message to disappear.)

I'm not sure what to do about this one. Perhaps we shouldn't worry about it considering that a "retry" can always be performed to get to messages that disappeared?

The intent of this refactoring is to consolidate references to the
various dependencies. Setting up a new dependency requires too much
wiring.
@petervdonovan petervdonovan force-pushed the check-versions branch 3 times, most recently from ed8607c to 97f312e Compare June 17, 2022 17:53
This adds detail to the button shown to the user, which should say
something like "Install using Snap" instead of just "Install." This
also unburdens the user from having to press Enter to run the suggested
command.
@petervdonovan
Copy link
Contributor Author

I still need to check in the GUI that this behaves as intended, but the tests pass, and I don't anticipate major changes in this PR before it gets merged.

@petervdonovan petervdonovan marked this pull request as ready for review June 17, 2022 21:19
The script was originally removed because it was difficult to update
dependencies without waiting for the LDS to build. Perhaps a better
workaround is to use the --install-dependencies flag (although that
does have known risks, since it also disables any install scripts that
our dependencies may have).
This repository does not need the action, but lf-lang/release-tools
does.
@petervdonovan
Copy link
Contributor Author

Another caveat: This PR introduces nondeterministic test failures like this:

Downloading VS Code 1.68.1 from https://update.code.visualstudio.com/1.68.1/darwin/stable
node:events:505
      throw er; // Unhandled 'error' event
      ^
Error: read ECONNRESET
    at TLSWrap.onStreamRead (node:internal/stream_base_commons:217:20)
Emitted 'error' event on ClientRequest instance at:
    at TLSSocket.socketErrorListener (node:_http_client:454:9)
    at TLSSocket.emit (node:events:527:28)
    at emitErrorNT (node:internal/streams/destroy:157:8)
    at emitErrorCloseNT (node:internal/streams/destroy:122:3)
    at processTicksAndRejections (node:internal/process/task_queues:83:21) {
  errno: -54,
  code: 'ECONNRESET',
  syscall: 'read'
}

These only occur on the MacOS runners, but they occur pretty often -- something like half the time. I think that these failures are bad, but acceptable.

@lhstrh
Copy link
Member

lhstrh commented Jun 17, 2022

Would caching be a good strategy to mitigate the connection issues?

These tests uncovered 3 (!!!) bugs (all fixed in this commit). Yikes.
src/version_checker.ts Outdated Show resolved Hide resolved
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

I have no tested this locally yet, but this looks great! I left some minor comments.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.vscode/launch.json Show resolved Hide resolved
.vscode/tasks.json Show resolved Hide resolved
src/build_commands.ts Outdated Show resolved Hide resolved
src/build_lds.ts Show resolved Hide resolved
@petervdonovan
Copy link
Contributor Author

After the latest bugfix, I just got this branch to pass a basic smoke test (by installing Rust from within the VS Code editor). I think that what we have now should be better than nothing.

@lhstrh lhstrh changed the title Check dependencies Dependency checks and environment setup assistance Jul 21, 2022
@petervdonovan petervdonovan merged commit 0de30fc into main Jul 22, 2022
@petervdonovan petervdonovan deleted the check-versions branch July 22, 2022 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants