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

bugfix: Make sure proxy settings are being forwarded to coursier #1539

Merged
merged 1 commit into from
Oct 22, 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
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,13 @@ describe("getJavaOptions", () => {
});

it("sanitizes options from .jvmopts", () => {
const malformedOptions = ["-XX:+UseNUMA ", "-XX:+UseZGC "];
const expectedOptions = ["-XX:+UseNUMA", "-XX:+UseZGC"];
const malformedOptions = [
"-Dhttp.proxyHost=proxy.example.com ",
"-Dhttp.proxyPort=1234 ",
];
const workspaceRoot = createWorskpace(malformedOptions);
const options = getJavaOptions(workspaceRoot);
expect(options).toEqual(expectedOptions);
expect(options).toEqual(proxyOptions);
});

it("reads from JAVA_OPTS", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ describe("setupCoursier", () => {
tmpDir,
process.cwd(),
new LogOutputChannel(),
false
false,
["-Xmx1000M", "-XX-fake!"]
);
expect(fs.existsSync(coursier)).toBeTruthy;
expect(fs.existsSync(javaHome)).toBeTruthy;
Expand Down
14 changes: 5 additions & 9 deletions packages/metals-languageclient/src/fetchMetals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as semver from "semver";
import { ChildProcessPromise, spawn } from "promisify-child-process";
import { JavaConfig } from "./getJavaConfig";
import { OutputChannel } from "./interfaces/OutputChannel";
import { convertToCoursierProperties } from "./getJavaOptions";

interface FetchMetalsOptions {
serverVersion: string;
Expand Down Expand Up @@ -70,15 +71,10 @@ export async function fetchMetals({
].concat(coursierArgs);
return { promise: spawn(javaPath, jarArgs, environment) };
} else {
// Convert Java properties to the "-J" argument form used by Coursier
var javaArgs: Array<string> = [];

// setting properties on windows native launcher doesn't work
if (process.platform != "win32")
javaArgs = javaOptions
.concat(["-Dfile.encoding=UTF-8"])
.concat(fetchProperties)
.map((p) => `-J${p}`);
const javaArgs: Array<string> = convertToCoursierProperties(
javaOptions.concat(["-Dfile.encoding=UTF-8"]).concat(fetchProperties),
false
);

return {
promise: spawn(coursier, javaArgs.concat(coursierArgs), environment),
Expand Down
21 changes: 21 additions & 0 deletions packages/metals-languageclient/src/getJavaOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,26 @@ function sanitizeOption(option: string): string {
return option.trim();
}

export function convertToCoursierProperties(
serverProperties: string[],
isCoursierJar: boolean
): string[] {
// setting properties on windows native launcher doesn't work
if (!isCoursierJar && process.platform == "win32") {
return [];
} else {
return serverProperties
.filter((prop) => isValidOption(prop))
.map((prop) => {
if (isCoursierJar) {
return prop;
} else {
return "-J" + prop;
}
});
}
}

function isValidOption(option: string): boolean {
return (
option.startsWith("-") &&
Expand All @@ -28,6 +48,7 @@ function isValidOption(option: string): boolean {
!option.startsWith("-Xms") &&
!option.startsWith("-Xmx") &&
!option.startsWith("-Xss") &&
!option.startsWith("-XX:") &&
// Do not alter stdout that we capture when using Coursier
option !== "-XX:+PrintCommandLineFlags"
);
Expand Down
23 changes: 20 additions & 3 deletions packages/metals-languageclient/src/setupCoursier.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { OutputChannel } from "./interfaces/OutputChannel";
import path from "path";
import fs from "fs";
import { findOnPath } from "./util";
import { convertToCoursierProperties } from "./getJavaOptions";

const coursierVersion = "v2.1.8";
// https://github.com/coursier/launchers contains only launchers with the most recent version
Expand All @@ -20,7 +21,8 @@ export async function setupCoursier(
coursierFetchPath: string,
extensionPath: string,
output: OutputChannel,
forceCoursierJar: boolean
forceCoursierJar: boolean,
serverProperties: string[]
): Promise<{ coursier: string; javaHome: string }> {
const handleOutput = (out: Buffer) => {
const msg = "\t" + out.toString().trim().split("\n").join("\n\t");
Expand Down Expand Up @@ -54,15 +56,30 @@ export async function setupCoursier(
};

const resolveJavaHomeWithCoursier = async (coursier: string) => {
const nonJvmServerProperties = convertToCoursierProperties(
serverProperties,
coursier.endsWith(".jar")
);
await run(
coursier,
["java", "--jvm", `temurin:${javaVersion}`, "-version"],
[
"java",
...nonJvmServerProperties,
"--jvm",
`temurin:${javaVersion}`,
"-version",
],
handleOutput
);

const getJavaPath = spawn(
coursier,
["java-home", "--jvm", `temurin:${javaVersion}`],
[
"java-home",
...nonJvmServerProperties,
"--jvm",
`temurin:${javaVersion}`,
],
{
encoding: "utf8",
}
Expand Down
3 changes: 2 additions & 1 deletion packages/metals-vscode/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,13 +230,14 @@
metalsDirPath,
context.extensionPath,
outputChannel,
forceCoursierJar
forceCoursierJar,
serverProperties
);

const canRetryWithJar =
javaHome && !coursier.endsWith(".jar") && !forceCoursierJar;

function retry(error: any): Promise<any> {

Check warning on line 240 in packages/metals-vscode/src/extension.ts

View workflow job for this annotation

GitHub Actions / Eslint

Unexpected any. Specify a different type

Check warning on line 240 in packages/metals-vscode/src/extension.ts

View workflow job for this annotation

GitHub Actions / Eslint

Unexpected any. Specify a different type
if (canRetryWithJar) {
outputChannel.appendLine(
"Trying again with the embedded coursier. This might take longer."
Expand Down Expand Up @@ -278,7 +279,7 @@
serverProperties,
javaConfig,
serverVersion
).catch((reason): Promise<any> => {

Check warning on line 282 in packages/metals-vscode/src/extension.ts

View workflow job for this annotation

GitHub Actions / Eslint

Unexpected any. Specify a different type
outputChannel.appendLine(
"Launching Metals failed with the following:"
);
Expand Down
Loading