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.

Sign up for GitHub

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

Transmit R session's LANG to the child process for the test runner #2488

Merged
merged 1 commit into from
Mar 20, 2024
Merged

Transmit R session's LANG to the child process for the test runner #2488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion extensions/positron-r/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,7 @@
},
"positron": {
"binaryDependencies": {
"ark": "0.1.66"
"ark": "0.1.67"
}
}
}
31 changes: 31 additions & 0 deletions extensions/positron-r/src/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ interface RPackageInstallation {
packageVersion?: string;
}

// At the time of writing, we only use LANG, but we expect other aspects of the active R session's
// locale to also be present here, such as LC_CTYPE or LC_TIME. These can vary by OS, so this
// interface doesn't attempt to enumerate them.
interface Locale {
LANG: string;
[key: string]: string;
}

/**
* A Positron language runtime that wraps a Jupyter kernel and a Language Server
* Protocol client.
Expand Down Expand Up @@ -313,6 +321,21 @@ export class RSession implements positron.LanguageRuntimeSession, vscode.Disposa
this._kernel?.showOutput();
}

/**
* Get the LANG env var and all categories of the locale, in R's Sys.getlocale() sense, from
* the R session.
*/
async getLocale(): Promise<Locale> {
try {
const locale: Locale = await this.callMethod('get_locale');
return locale;
} catch (err) {
const runtimeError = err as positron.RuntimeMethodError;
throw new Error(`Error getting locale information: ${runtimeError.message} ` +
`(${runtimeError.code})`);
}
}

/**
* Checks whether a package is installed in the runtime.
* @param pkgName The name of the package to check
Expand Down Expand Up @@ -670,3 +693,11 @@ export async function checkInstalled(pkgName: string,
}
throw new Error(`Cannot check install status of ${pkgName}; no R session available`);
}

export async function getLocale(session?: RSession): Promise<Locale> {
session = session || RSessionManager.instance.getConsoleSession();
if (session) {
return session.getLocale();
}
throw new Error(`Cannot get locale information; no R session available`);
}
26 changes: 11 additions & 15 deletions extensions/positron-r/src/testing/runner-testthat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import * as path from 'path';
import { spawn } from 'child_process';
import * as split2 from 'split2';
import { LOGGER } from '../extension';
import { checkInstalled } from '../session';
import { checkInstalled, getLocale } from '../session';
import { EXTENSION_ROOT_DIR } from '../constants';
import { ItemType, TestingTools, encodeNodeId } from './util-testing';
import { TestResult } from './reporter';
Expand Down Expand Up @@ -97,24 +97,20 @@ export async function runThatTest(

const wd = testingTools.packageRoot.fsPath;
LOGGER.info(`Running devtools call in working directory ${wd}`);
const locale = await getLocale();
Copy link
Member Author

Choose a reason for hiding this comment

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

Typical value for locale on macOS;

{
  LANG: "en_US.UTF-8",
  LC_ALL: "en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8",
  LC_COLLATE: "en_US.UTF-8",
  LC_CTYPE: "en_US.UTF-8",
  LC_MONETARY: "en_US.UTF-8",
  LC_NUMERIC: "C",
  LC_TIME: "en_US.UTF-8",
  LC_MESSAGES: "en_US.UTF-8",
  LC_PAPER: "",
  LC_MEASUREMENT: "",
}

Copy link
Member Author

Choose a reason for hiding this comment

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

And here's how it looks in my Windows VM:

2024-03-19 14:17:47.351 [info] Locale info from active R session: {
  "LANG": "en_US.UTF-8",
  "LC_ALL": "LC_COLLATE=English_United States.utf8;LC_CTYPE=English_United States.utf8;LC_MONETARY=English_United States.utf8;LC_NUMERIC=C;LC_TIME=English_United States.utf8",
  "LC_COLLATE": "English_United States.utf8",
  "LC_CTYPE": "English_United States.utf8",
  "LC_MONETARY": "English_United States.utf8",
  "LC_NUMERIC": "C",
  "LC_TIME": "English_United States.utf8",
  "LC_MESSAGES": "",
  "LC_PAPER": "",
  "LC_MEASUREMENT": ""
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, here is what I see:

2024-03-20 13:15:59.975 [info] Locale info from active R session: {
  "LANG": "en_US.UTF-8",
  "LC_ALL": "en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8",
  "LC_COLLATE": "en_US.UTF-8",
  "LC_CTYPE": "en_US.UTF-8",
  "LC_MONETARY": "en_US.UTF-8",
  "LC_NUMERIC": "C",
  "LC_TIME": "en_US.UTF-8",
  "LC_MESSAGES": "en_US.UTF-8",
  "LC_PAPER": "",
  "LC_MEASUREMENT": ""
}

LOGGER.info(`Locale info from active R session: ${JSON.stringify(locale, null, 2)}`);
let hostFile = '';
// TODO @jennybc: if this code stays, figure this out
// eslint-disable-next-line no-async-promise-executor
return new Promise<string>(async (resolve, reject) => {
// FIXME (@jennybc): once I can ask the current runtime for its LC_CTYPE (and possibly
// other locale categories or even LANG), use something like this to make the child
// process better match the runtime. Learned this from reprex's UTF-8 test which currently
// fails in the test explorer because the reprex is being rendered in the C locale.
// Also affects the tests for glue.
// const childProcess = spawn(command, {
// cwd: wd,
// shell: true,
// env: {
// ...process.env,
// LC_CTYPE: 'en_US.UTF-8'
// }
// });
const childProcess = spawn(command, { cwd: wd, shell: true });
const childProcess = spawn(command, {
cwd: wd,
shell: true,
env: {
...process.env,
LANG: locale['LANG']
Copy link
Member Author

Choose a reason for hiding this comment

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

I originally thought I would set individual components, such as LC_CTYPE and LC_TIME, but reading about related child_process.spawn() + locale issues in VS Code and extensions suggested that LANG is the best place to intervene. It appears to lead to correct values of other aspects of the locale and strikes me as the cleanest solution.

}
});
let stdout = '';
const testStartDates = new WeakMap<vscode.TestItem, number>();
childProcess.stdout!
Expand Down
Loading