From c2f3f8cb486edb079d129db0c08b53535f88a20c Mon Sep 17 00:00:00 2001 From: Adrian Vogelsgesang Date: Fri, 1 Mar 2024 17:06:09 +0100 Subject: [PATCH] Support running buildifier through Bazel (#350) Buildifier is frequently installed through Bazel as described on https://github.com/bazelbuild/buildtools/blob/master/buildifier/README.md#setup-and-usage-via-bazel such that it can be run through `bazel run`. This commit allows the `bazel.buildifierExecutable` setting to refer to a Bazel target. All paths starting with `@` are interpreted as Bazel target names and are executed through `bazel run`. Bazel targets could also start with `//` but we interpret those as normal file system paths. If someone wants to run a target in the own workspace, they can simply use, e.g., `@//:buildifier` instead of `//:buildifier`. As a drive-by fix, I also fixed #329 since it was a one-line fix. Fixes #185, #329 Co-authored-by: John Firebaugh --- package.json | 2 +- src/buildifier/buildifier.ts | 18 ++++++- src/buildifier/buildifier_availability.ts | 62 +++++++++++------------ 3 files changed, 48 insertions(+), 34 deletions(-) diff --git a/package.json b/package.json index f4a5bfb9..bec3d733 100644 --- a/package.json +++ b/package.json @@ -118,7 +118,7 @@ "bazel.buildifierExecutable": { "type": "string", "default": "", - "markdownDescription": "The name of the Buildifier executable. This may be an absolute path, or a simple name that will be searched for on the system path. If empty, \"buildifier\" on the system path will be used.\n\nBuildifier can be downloaded from https://github.com/bazelbuild/buildtools/releases.", + "markdownDescription": "The name of the Buildifier executable. This may be an absolute path, or a simple name that will be searched for on the system path. Paths starting with `@` are interpreted as Bazel targets and are run via `bazel run`. If empty, \"buildifier\" on the system path will be used.\n\nBuildifier can be downloaded from https://github.com/bazelbuild/buildtools/releases.", "scope": "machine-overridable" }, "bazel.buildifierFixOnFormat": { diff --git a/src/buildifier/buildifier.ts b/src/buildifier/buildifier.ts index c9192a5c..99746a8c 100644 --- a/src/buildifier/buildifier.ts +++ b/src/buildifier/buildifier.ts @@ -16,6 +16,7 @@ import * as child_process from "child_process"; import * as path from "path"; import * as vscode from "vscode"; import { IBuildifierResult, IBuildifierWarning } from "./buildifier_result"; +import { getDefaultBazelExecutablePath } from "../extension/configuration"; /** Whether to warn about lint findings or fix them. */ export type BuildifierLintMode = "fix" | "warn"; @@ -182,17 +183,30 @@ export function getDefaultBuildifierExecutablePath(): string { * @param acceptNonSevereErrors If true, syntax/lint exit codes will not be * treated as severe tool errors. */ -function executeBuildifier( +export function executeBuildifier( fileContent: string, args: string[], acceptNonSevereErrors: boolean, ): Promise<{ stdout: string; stderr: string }> { return new Promise((resolve, reject) => { + // Determine the executable + let executable = getDefaultBuildifierExecutablePath(); + // Paths starting with an `@` are referring to Bazel targets + if (executable.startsWith("@")) { + const targetName = executable; + executable = getDefaultBazelExecutablePath(); + args = ["run", targetName, "--", ...args]; + } const execOptions = { maxBuffer: Number.MAX_SAFE_INTEGER, + // Use the workspace folder as CWD, thereby allowing relative + // paths. See #329 + cwd: vscode.workspace.workspaceFolders?.[0]?.uri?.fsPath, }; + + // Start buildifier const process = child_process.execFile( - getDefaultBuildifierExecutablePath(), + executable, args, execOptions, (error, stdout, stderr) => { diff --git a/src/buildifier/buildifier_availability.ts b/src/buildifier/buildifier_availability.ts index 9821545b..23e7031c 100644 --- a/src/buildifier/buildifier_availability.ts +++ b/src/buildifier/buildifier_availability.ts @@ -12,11 +12,13 @@ // See the License for the specific language governing permissions and // limitations under the License. -import * as child_process from "child_process"; import * as vscode from "vscode"; import * as which from "which"; -import { getDefaultBuildifierExecutablePath } from "./buildifier"; +import { + executeBuildifier, + getDefaultBuildifierExecutablePath, +} from "./buildifier"; /** The URL to load for buildifier's releases. */ const BUILDTOOLS_RELEASES_URL = @@ -31,36 +33,34 @@ const BUILDTOOLS_RELEASES_URL = */ export async function checkBuildifierIsAvailable() { const buildifierExecutable = getDefaultBuildifierExecutablePath(); - await which(buildifierExecutable) - .catch(async () => { + // Check if the program exists (in case it's an actual executable and not + // an target name starting with `@`). + if (!buildifierExecutable.startsWith("@")) { + try { + await which(buildifierExecutable); + } catch (e) { await showBuildifierDownloadPrompt("Buildifier was not found"); - }) - .then(() => { - // If we found it, make sure it's a compatible version by running - // buildifier on an empty input and see if it exits successfully and the - // output parses. - const process = child_process.execFile( - buildifierExecutable, - ["--format=json", "--mode=check"], - {}, - (error: Error, stdout: string) => { - if (!error && stdout) { - try { - JSON.parse(stdout); - return; - } catch { - // Swallow the error; we'll display the prompt below. - } - } - // If no valid JSON back, we don't have a compatible version. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - showBuildifierDownloadPrompt( - "Buildifier is too old (0.25.1 or higher is needed)", - ); - }, - ); - process.stdin.end(); - }); + return; + } + } + + // Make sure it's a compatible version by running + // buildifier on an empty input and see if it exits successfully and the + // output parses. + const { stdout } = await executeBuildifier( + "", + ["--format=json", "--mode=check"], + false, + ); + try { + JSON.parse(stdout); + } catch { + // If we got no valid JSON back, we don't have a compatible version. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + showBuildifierDownloadPrompt( + "Buildifier is too old (0.25.1 or higher is needed)", + ); + } } /**