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

Package manager nullability fixes #5255

Merged
merged 24 commits into from
Jul 1, 2022

Conversation

50Wliu
Copy link
Contributor

@50Wliu 50Wliu commented Jun 5, 2022

Covers:

  • src/packageManager/
  • src/razor/
  • src/tools/

Notable changes:

  • Added missing architectures to macOS x64 OmniSharp download (I hope this wasn't intentional to allow M1 Macs to download both ARM64 + x64 OmniSharps? architectures is marked as non-null in IPackage).
  • Remove all guards that checked for non-null packages, as there is a root check for that when initially reading runtimeDependencies from package.json.
    • This is slightly risky as it's possible I missed a root caller that doesn't check that runtimeDependencies from an any-typed package.json is non-null - but I mean, with the previous behavior we would just silently refuse to download OmniSharp. At least now there will be an obvious error thrown somewhere.
  • Similarly, removed all version checks as there's a root semver.valid(version) check.
  • downloadAndInstallPackages is only ever called with a list of validated OmniSharp packages (function should be renamed to reflect that...), so removed some unnecessary checks around that.

While we're here, I'd like to understand the current state of how runtimeDependencies are defined/parsed, so that I have the context to create a followup PR cleaning up some of the associated logic.

  • Is installTestPath used for anything? There's 0 references from AbsolutePathPackage and I don't see any meaningful reads from IPackage.
  • Why do the packages in package.json specify a URL + installPath, if they're just overriden in OmnisharpPackageCreator (albeit, to the same thing) from constants in server.ts?
    • Should we stop overriding in OmnisharpPackageCreator, or stop including the URL + installPath in package.json?
  • fallbackUrl is never defined in any of the packages in package.json. Is this something that should be kept around?

@50Wliu 50Wliu force-pushed the users/winstonliu/misc-nullability branch from e59609d to 5b8983c Compare June 5, 2022 04:41
Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

Thanks @50Wliu! Sorry it took me so long to review.

}

if (pkg.url === null) {
eventStream.post(new InstallationFailure(installationStage, new Error("A release package of OmniSharp does not exist for this platform. Set \"omnisharp.path\" to \"latest\" in Settings to use an experimental build.")));
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for removing this.

@JoeRobich JoeRobich merged commit 1792ad5 into dotnet:master Jul 1, 2022
@50Wliu 50Wliu deleted the users/winstonliu/misc-nullability branch July 7, 2022 05:30
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