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

feat: More ways to specify R path #64

Merged
merged 6 commits into from
Aug 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Change Log

## [UNRELEASED]

* In Positron, the extension now uses the selected R runtime for Shiny for R apps. In VS Code, the extension also now consults the `r.rpath.mac`, `r.rpath.windows` or `r.rpath.linux` settings to find the R executable, before falling back to system settings. These settings are part of the [R Debugger extension](https://marketplace.visualstudio.com/items?itemName=RDebugger.r-debugger) ([#64](https://github.com/posit-dev/shiny-vscode/pull/64))

## 1.0.0

The Shiny extension for VS Code now has a new extension ID: `Posit.shiny`! New Shiny users should install the Shiny extension from [the VS Code marketplace](https://marketplace.visualstudio.com/items?itemName=Posit.shiny) or [https://open-vsx.org/extension/posit/shiny](https://open-vsx.org/extension/posit/shiny).
Expand Down
52 changes: 52 additions & 0 deletions src/extension-api-utils/extensionHost.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import * as vscode from "vscode";

declare const globalThis: {
[key: string]: any;
};

export interface HostWebviewPanel extends vscode.Disposable {
readonly webview: vscode.Webview;
readonly visible: boolean;
reveal(viewColumn?: vscode.ViewColumn, preserveFocus?: boolean): void;
readonly onDidChangeViewState: vscode.Event<any>;
readonly onDidDispose: vscode.Event<void>;
}

type LanguageRuntimeMetadata = Partial<{
// https://github.com/posit-dev/positron/blob/39a01b71/src/positron-dts/positron.d.ts#L357
/** The path to the runtime. */
runtimePath: string;

/**
* The fully qualified name of the runtime displayed to the user; e.g. "R 4.2 (64-bit)".
* Should be unique across languages.
*/
runtimeName: string;

/**
* A language specific runtime name displayed to the user; e.g. "4.2 (64-bit)".
* Should be unique within a single language.
*/
runtimeShortName: string;

/** The version of the runtime itself (e.g. kernel or extension version) as a string; e.g. "0.1" */
runtimeVersion: string;
}>;

export function getExtensionHostPreview(): void | ((url: string) => HostWebviewPanel) {
const pst = getPositronAPI();
if (!pst) { return; }
return (url: string) => pst.window.previewUrl(vscode.Uri.parse(url));
}

export async function getPositronPreferredRuntime(languageId: string): Promise<LanguageRuntimeMetadata | undefined> {
const pst = getPositronAPI();
if (!pst) { return; }
return await pst.runtime.getPreferredRuntime(languageId);
}

function getPositronAPI(): undefined | any {
if (typeof globalThis?.acquirePositronApi !== "function") { return; }

return globalThis.acquirePositronApi();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just out of curiosity, not asking for a change... Is this all equivalent to globalThis.acquirePositronApi?()? (Other than you're gracefully dealing with globalThis not being an object and acquirePositronApi not being a function, both of which seem unlikely?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean globalThis?.acquirePositronApi(), right? Then yes, it's equivalent if overly cautious. I'm happy to switch to the cleaner syntax.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, I don't think that's right... I think globalThis?. checks for globalThis being null/undefined but not acquirePositronApi. It looks to me like ?() is not a thing, I think maybe the original is best.

Or

const apa = globalThis?.acquirePositronApi;
return apa ? apa() : undefined;

Or

if (typeof acquirePositronApi === "function") {
  return acquirePositronApi();
}

(Bikeshedding so hard right now... sorry... 😬)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or

globalThis?.acquirePositronApi?.call(globalThis)

(Alright I'm done, I promise!)

Copy link
Collaborator Author

@gadenbuie gadenbuie Jul 22, 2024

Choose a reason for hiding this comment

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

I don't think that's right... I think globalThis?. checks for globalThis being null/undefined but not acquirePositronApi

Oh yeah, that's right. I think this is pretty succinct and brings in the right ideas:

function getPositronAPI(): undefined | any {
    if (typeof globalThis?.acquirePositronApi !== "function") { return; }

    return globalThis.acquirePositronApi();
}

}
24 changes: 0 additions & 24 deletions src/extension-api-utils/extensionHostPreview.ts

This file was deleted.

2 changes: 1 addition & 1 deletion src/net-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { AddressInfo } from "net";
import * as vscode from "vscode";
import { getRemoteSafeUrl } from "./extension-api-utils/getRemoteSafeUrl";
import { retryUntilTimeout } from "./retry-utils";
import { getExtensionHostPreview } from "./extension-api-utils/extensionHostPreview";
import { getExtensionHostPreview } from "./extension-api-utils/extensionHost";

/**
* Tests if a port is open on a host, by trying to connect to it with a TCP
Expand Down
52 changes: 51 additions & 1 deletion src/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
} from "./shell-utils";
import { getAppPort, getAutoreloadPort } from "./port-settings";
import * as winreg from "winreg";
import { getPositronPreferredRuntime } from "./extension-api-utils/extensionHost";

const DEBUG_NAME = "Debug Shiny app";

Expand Down Expand Up @@ -344,7 +345,56 @@
}

async function getRBinPath(bin: string): Promise<string> {
return getRPathFromEnv(bin) || (await getRPathFromWindowsReg(bin)) || "";
return (
(await getRPathFromPositron(bin)) ||
getRPathFromConfig(bin) ||
getRPathFromEnv(bin) ||
(await getRPathFromWindowsReg(bin)) ||
""
);
}

async function getRPathFromPositron(bin: string): Promise<string> {
const runtimeMetadata = await getPositronPreferredRuntime("r");
if (!runtimeMetadata) {
return "";
}

console.log(`[shiny] runtimeMetadata: ${JSON.stringify(runtimeMetadata)}`)

Check warning on line 363 in src/run.ts

View workflow job for this annotation

GitHub Actions / build-vsix

Missing semicolon

const runtimePath = runtimeMetadata.runtimePath;
if (!runtimePath) {
return "";
}

const { platform } = process;
const fileExt = platform === "win32" ? ".exe" : "";
return path_join(path_dirname(runtimePath), bin + fileExt);
}

function getRPathFromConfig(bin: string): string {
const { platform } = process;
const fileExt = platform === "win32" ? ".exe" : "";
let osType: string;

switch (platform) {
case "win32":
osType = "windows";
break;
case "darwin":
osType = "mac";
break;
default:
osType = "linux";
}

const rPath = vscode.workspace
.getConfiguration("r.rpath")
.get(osType, undefined);

console.log(`[shiny] rPath: ${rPath}`);

return rPath ? path_join(path_dirname(rPath), bin + fileExt) : "";
}

function getRPathFromEnv(bin: string = "R"): string {
Expand Down