Closed srchase closed 1 year ago
Do you mind if we also support the env variable interpolation if it's not a lot of extra code? That feature would be of most use to my team, and I can have someone assigned to contribute it (with support for both ways).
Ideally, we're able to re-use as much of the Maven dependency resolution from smithy-build as possible. The Language Server's MavenConfig and MavenRepository model classes should get removed in favor of the equivalents from smithy-build's model classes. I think there's an open question on how DependencyDownloader would get updated and how much it continues to rely on Coursier's Fetch API, or if any of that goes away in favor of how the Smithy CLI fetches the same dependencies.
I put up this branch to start using the CLI's resolvers: https://github.com/srchase/smithy-language-server/tree/use-smithy-cli-maven-resolution
It needs tests, and SmithyBuildExtensions needs updated to wrap a SmithyBuildConfig, so it can get variable interpolation for the Maven config like SmithyBuildConfig has. That shouldn't be too much more work, and then I can get cut a PR.
That's awesome. Happy to review and give it a test when the PR is out!
I pushed some additional changes to the branch.
In testing this change with the VS Code extension, I ran into two small things on the extension side:
cs launch software.amazon.smithy:smithy-language-server:0.2.3 -r m2local -M software.amazon.smithy.lsp.Main -- 12345
code
command, it properly inherits the shell's environment variables:
code ~/path/to/my/workspace
. If VS Code is not launched from a shell, it won't pick up the environment vars, SMITHY_MAVEN_REPOS
, etc. that the LSP may need for resolving dependencies, or interpolating credentials.Now that the Smithy CLI is pulled in, Coursier finds the wrong main class. The LSP's main class needs set explicitly
Interesting, I assumed it would ignore main classes from dependencies. Perhaps the Smithy CLI could be decomposed into smaller modules, so that the one without a main class can be used.
When VS Code is launched with the code command, it properly inherits the shell's environment variables: code ~/path/to/my/workspace. If VS Code is not launched from a shell, it won't pick up the environment vars, SMITHY_MAVEN_REPOS, etc. that the LSP may need for resolving dependencies, or interpolating credentials.
Yeah, I've hit that in the past, tbh it's quite concerning. I'm thinking an ability to load a properties/env file would be helpful here, but perhaps there's a better solution. edit: actually, that doesn't help much with my current setup, we sort of rely on people having the env vars populated in zshrc or something similar... so maybe it's not really that much of an issue that you need to launch Code from a shell.
Currently, Maven repositories can be configured in a smithy-build.json file, but the Language Server does not support setting
httpCredentials
with environment variable expansion the way that smithy-build or the Smithy CLI can. See: https://smithy.io/2.0/guides/building-models/build-config.html#maven-repositoriesBefore the Language Server is updated to use the same dependency resolution as the Smithy CLI, a smaller change would be to support the
SMITHY_MAVEN_REPOS
environment variable so that username and passwords for Maven repositories can be used in a shared way between smithy-build and the Language Server. See: https://smithy.io/2.0/guides/building-models/build-config.html#smithy-maven-repos-environment-variable