Skip to content

Commit e56977b

Browse files
Almighty-AlpacaAlmighty-Alpaca
authored
fix: don't assume commands end with .cmd on Windows (#9026)
1 parent d13bfdb commit e56977b

File tree

14 files changed

+142
-143
lines changed

14 files changed

+142
-143
lines changed

‎.changeset/long-poems-speak.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"app-builder-lib": patch
3+
---
4+
5+
fix: don't assume commands end with .cmd on Windows by leveraging `which` package

‎packages/app-builder-lib/package.json

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,8 @@
7878
"semver": "^7.3.8",
7979
"tar": "^6.1.12",
8080
"temp-file": "^3.4.0",
81-
"tiny-async-pool": "1.3.0"
81+
"tiny-async-pool": "1.3.0",
82+
"which": "^5.0.0"
8283
},
8384
"///": "babel in devDependencies for proton tests",
8485
"devDependencies": {
@@ -109,6 +110,7 @@
109110
"@types/semver": "7.3.8",
110111
"@types/tar": "^6.1.3",
111112
"@types/tiny-async-pool": "^1",
113+
"@types/which": "^3.0.4",
112114
"dmg-builder": "workspace:*",
113115
"electron-builder-squirrel-windows": "workspace:*",
114116
"toml": "^3.0.0"

‎packages/app-builder-lib/src/node-module-collector/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { NodeModuleInfo } from "./types"
66
import { exec } from "builder-util"
77

88
async function isPnpmProjectHoisted(rootDir: string) {
9-
const command = await PnpmNodeModulesCollector.pmCommand.value
9+
const command = getPackageManagerCommand(PM.PNPM)
1010
const config = await exec(command, ["config", "list"], { cwd: rootDir, shell: true })
1111
const lines = Object.fromEntries(config.split("\n").map(line => line.split("=").map(s => s.trim())))
1212
return lines["node-linker"] === "hoisted"

‎packages/app-builder-lib/src/node-module-collector/nodeModulesCollector.ts

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import * as path from "path"
33
import * as fs from "fs"
44
import type { NodeModuleInfo, DependencyGraph, Dependency } from "./types"
55
import { exec, log } from "builder-util"
6-
import { Lazy } from "lazy-val"
6+
import { getPackageManagerCommand, PM } from "./packageManager"
77

88
export abstract class NodeModulesCollector<T extends Dependency<T, OptionalsType>, OptionalsType> {
99
private nodeModules: NodeModuleInfo[] = []
@@ -24,19 +24,18 @@ export abstract class NodeModulesCollector<T extends Dependency<T, OptionalsType
2424
return this.nodeModules
2525
}
2626

27-
public abstract readonly installOptions: Promise<{
28-
cmd: string
29-
args: string[]
27+
public abstract readonly installOptions: {
28+
manager: PM
3029
lockfile: string
31-
}>
32-
protected abstract readonly pmCommand: Lazy<string>
30+
}
31+
3332
protected abstract getArgs(): string[]
3433
protected abstract parseDependenciesTree(jsonBlob: string): T
3534
protected abstract extractProductionDependencyGraph(tree: Dependency<T, OptionalsType>, dependencyId: string): void
3635
protected abstract collectAllDependencies(tree: Dependency<T, OptionalsType>): void
3736

3837
protected async getDependenciesTree(): Promise<T> {
39-
const command = await this.pmCommand.value
38+
const command = getPackageManagerCommand(this.installOptions.manager)
4039
const args = this.getArgs()
4140
const dependencies = await exec(command, args, {
4241
cwd: this.rootDir,

‎packages/app-builder-lib/src/node-module-collector/npmNodeModulesCollector.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,13 @@
1-
import { Lazy } from "lazy-val"
21
import { NodeModulesCollector } from "./nodeModulesCollector"
2+
import { PM } from "./packageManager"
33
import { NpmDependency } from "./types"
44

55
export class NpmNodeModulesCollector extends NodeModulesCollector<NpmDependency, string> {
66
constructor(rootDir: string) {
77
super(rootDir)
88
}
99

10-
public readonly pmCommand = new Lazy<string>(() => Promise.resolve(process.platform === "win32" ? "npm.cmd" : "npm"))
11-
public readonly installOptions = this.pmCommand.value.then(cmd => ({ cmd, args: ["ci"], lockfile: "package-lock.json" }))
10+
public readonly installOptions = { manager: PM.NPM, lockfile: "package-lock.json" }
1211

1312
protected getArgs(): string[] {
1413
return ["list", "-a", "--include", "prod", "--include", "optional", "--omit", "dev", "--json", "--long", "--silent"]

‎packages/app-builder-lib/src/node-module-collector/packageManager.ts

Lines changed: 52 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import * as path from "path"
22
import * as fs from "fs"
3+
import * as which from "which"
34

45
export enum PM {
56
NPM = "npm",
@@ -8,96 +9,77 @@ export enum PM {
89
YARN_BERRY = "yarn-berry",
910
}
1011

11-
function detectPackageManagerByEnv(): PM {
12-
if (process.env.npm_config_user_agent) {
13-
const userAgent = process.env.npm_config_user_agent
14-
15-
if (userAgent.includes("pnpm")) {
16-
return PM.PNPM
17-
}
18-
19-
if (userAgent.includes("yarn")) {
20-
if (userAgent.includes("yarn/")) {
21-
const version = userAgent.match(/yarn\/(\d+)\./)
22-
if (version && parseInt(version[1]) >= 2) {
23-
return PM.YARN_BERRY
24-
}
25-
}
26-
return PM.YARN
27-
}
28-
29-
if (userAgent.includes("npm")) {
30-
return PM.NPM
31-
}
32-
}
12+
// Cache for resolved paths
13+
const pmPathCache: Record<PM, string | null | undefined> = {
14+
[PM.NPM]: undefined,
15+
[PM.YARN]: undefined,
16+
[PM.PNPM]: undefined,
17+
[PM.YARN_BERRY]: undefined,
18+
}
3319

34-
if (process.env.npm_execpath) {
35-
const execPath = process.env.npm_execpath.toLowerCase()
20+
function resolveCommand(pm: PM): string {
21+
const fallback = pm === PM.YARN_BERRY ? "yarn" : pm
3622

37-
if (execPath.includes("pnpm")) {
38-
return PM.PNPM
39-
}
23+
if (process.platform !== "win32") {
24+
return fallback
25+
}
4026

41-
if (execPath.includes("yarn")) {
42-
if (execPath.includes("berry") || process.env.YARN_VERSION?.startsWith("2.") || process.env.YARN_VERSION?.startsWith("3.")) {
43-
return PM.YARN_BERRY
44-
}
45-
return PM.YARN
46-
}
27+
try {
28+
return path.basename(which.sync(fallback))
29+
} catch {
30+
// If `which` fails (not found), still return the fallback string
31+
return fallback
32+
}
33+
}
4734

48-
if (execPath.includes("npm")) {
49-
return PM.NPM
50-
}
35+
export function getPackageManagerCommand(pm: PM) {
36+
if (pmPathCache[pm] !== undefined) {
37+
return pmPathCache[pm]!
5138
}
5239

53-
if (process.env.PNPM_HOME) {
40+
const resolved = resolveCommand(pm)
41+
pmPathCache[pm] = resolved
42+
return resolved
43+
}
44+
45+
export function detectPackageManagerByEnv(): PM {
46+
const ua = process.env.npm_config_user_agent ?? ""
47+
const execPath = process.env.npm_execpath?.toLowerCase() ?? ""
48+
49+
const yarnVersion = process.env.YARN_VERSION
50+
const isBerry = yarnVersion?.startsWith("2.") || yarnVersion?.startsWith("3.")
51+
52+
if (ua.includes("pnpm") || execPath.includes("pnpm") || process.env.PNPM_HOME) {
5453
return PM.PNPM
5554
}
5655

57-
if (process.env.YARN_REGISTRY) {
58-
if (process.env.YARN_VERSION?.startsWith("2.") || process.env.YARN_VERSION?.startsWith("3.")) {
59-
return PM.YARN_BERRY
60-
}
61-
return PM.YARN
56+
if (ua.includes("yarn") || execPath.includes("yarn") || process.env.YARN_REGISTRY) {
57+
return isBerry || ua.includes("yarn/2") || ua.includes("yarn/3") ? PM.YARN_BERRY : PM.YARN
6258
}
6359

64-
if (process.env.npm_package_json) {
60+
if (ua.includes("npm") || execPath.includes("npm") || process.env.npm_package_json) {
6561
return PM.NPM
6662
}
6763

68-
// return default
6964
return PM.NPM
7065
}
7166

72-
export function getPackageManagerCommand(pm: PM) {
73-
let cmd = pm
74-
if (pm === PM.YARN_BERRY || process.env.FORCE_YARN === "true") {
75-
cmd = PM.YARN
76-
}
77-
return `${cmd}${process.platform === "win32" ? ".cmd" : ""}`
78-
}
67+
export function detectPackageManager(cwd: string): PM {
68+
const has = (file: string) => fs.existsSync(path.join(cwd, file))
7969

80-
export function detectPackageManager(cwd: string) {
81-
const isYarnLockFileExists = fs.existsSync(path.join(cwd, "yarn.lock"))
82-
const isPnpmLockFileExists = fs.existsSync(path.join(cwd, "pnpm-lock.yaml"))
83-
const isNpmLockFileExists = fs.existsSync(path.join(cwd, "package-lock.json"))
84-
85-
if (isYarnLockFileExists && !isPnpmLockFileExists && !isNpmLockFileExists) {
86-
// check if yarn is berry
87-
const pm = detectPackageManagerByEnv()
88-
if (pm === PM.YARN_BERRY) {
89-
return PM.YARN_BERRY
90-
}
91-
return PM.YARN
92-
}
70+
const yarn = has("yarn.lock")
71+
const pnpm = has("pnpm-lock.yaml")
72+
const npm = has("package-lock.json")
9373

94-
if (isPnpmLockFileExists && !isYarnLockFileExists && !isNpmLockFileExists) {
95-
return PM.PNPM
96-
}
74+
const detected: PM[] = []
75+
if (yarn) detected.push(PM.YARN)
76+
if (pnpm) detected.push(PM.PNPM)
77+
if (npm) detected.push(PM.NPM)
9778

98-
if (isNpmLockFileExists && !isYarnLockFileExists && !isPnpmLockFileExists) {
99-
return PM.NPM
79+
if (detected.length === 1) {
80+
return detected[0] === PM.YARN ? detectPackageManagerByEnv() : detected[0]
10081
}
101-
// if there are no lock files or multiple lock files, return the package manager from env
82+
83+
// fallback: multiple lockfiles or none
10284
return detectPackageManagerByEnv()
10385
}

‎packages/app-builder-lib/src/node-module-collector/pnpmNodeModulesCollector.ts

Lines changed: 6 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,29 +1,16 @@
1-
import { Lazy } from "lazy-val"
2-
import { NodeModulesCollector } from "./nodeModulesCollector"
3-
import { PnpmDependency, Dependency } from "./types"
4-
import { exec, log } from "builder-util"
5-
import * as path from "path"
1+
import { log } from "builder-util"
62
import * as fs from "fs"
3+
import * as path from "path"
4+
import { NodeModulesCollector } from "./nodeModulesCollector"
5+
import { PM } from "./packageManager"
6+
import { Dependency, PnpmDependency } from "./types"
77

88
export class PnpmNodeModulesCollector extends NodeModulesCollector<PnpmDependency, PnpmDependency> {
99
constructor(rootDir: string) {
1010
super(rootDir)
1111
}
1212

13-
static readonly pmCommand = new Lazy<string>(async () => {
14-
if (process.platform === "win32") {
15-
try {
16-
await exec("pnpm", ["--version"])
17-
} catch (_error: any) {
18-
log.debug(null, "pnpm not detected, falling back to pnpm.cmd")
19-
return "pnpm.cmd"
20-
}
21-
}
22-
return "pnpm"
23-
})
24-
25-
protected readonly pmCommand: Lazy<string> = PnpmNodeModulesCollector.pmCommand
26-
public readonly installOptions = this.pmCommand.value.then(cmd => ({ cmd, args: ["install", "--frozen-lockfile"], lockfile: "pnpm-lock.yaml" }))
13+
public readonly installOptions = { manager: PM.PNPM, lockfile: "pnpm-lock.yaml" }
2714

2815
protected getArgs(): string[] {
2916
return ["list", "--prod", "--json", "--depth", "Infinity"]

‎packages/app-builder-lib/src/node-module-collector/yarnNodeModulesCollector.ts

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,11 @@
11
import { NpmNodeModulesCollector } from "./npmNodeModulesCollector"
2+
import { PM } from "./packageManager"
23

34
export class YarnNodeModulesCollector extends NpmNodeModulesCollector {
45
constructor(rootDir: string) {
56
super(rootDir)
67
}
78

8-
public readonly installOptions = Promise.resolve({
9-
cmd: process.platform === "win32" ? "yarn.cmd" : "yarn",
10-
args: ["install", "--frozen-lockfile"],
11-
lockfile: "yarn.lock",
12-
})
9+
// note: do not override instance-var `pmCommand`. We explicitly use npm for the json payload
10+
public readonly installOptions = { manager: PM.YARN, lockfile: "yarn.lock" }
1311
}

‎packages/app-builder-lib/src/util/yarn.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { executeAppBuilderAndWriteJson } from "./appBuilder"
1010
import { PM, detectPackageManager, getPackageManagerCommand } from "../node-module-collector"
1111
import { NodeModuleDirInfo } from "./packageDependencies"
1212
import { rebuild as remoteRebuild } from "./rebuild/rebuild"
13+
import * as which from "which"
1314

1415
export async function installOrRebuild(config: Configuration, { appDir, projectDir }: DirectoryPaths, options: RebuildOptions, forceInstall = false) {
1516
const effectiveOptions: RebuildOptions = {
@@ -78,7 +79,7 @@ export function getGypEnv(frameworkInfo: DesktopFrameworkInfo, platform: NodeJS.
7879
}
7980
}
8081

81-
async function installDependencies(config: Configuration, { appDir, projectDir }: DirectoryPaths, options: RebuildOptions): Promise<any> {
82+
export async function installDependencies(config: Configuration, { appDir, projectDir }: DirectoryPaths, options: RebuildOptions): Promise<any> {
8283
const platform = options.platform || process.platform
8384
const arch = options.arch || process.arch
8485
const additionalArgs = options.additionalArgs
@@ -114,7 +115,7 @@ async function installDependencies(config: Configuration, { appDir, projectDir }
114115
export async function nodeGypRebuild(platform: NodeJS.Platform, arch: string, frameworkInfo: DesktopFrameworkInfo) {
115116
log.info({ platform, arch }, "executing node-gyp rebuild")
116117
// this script must be used only for electron
117-
const nodeGyp = `node-gyp${process.platform === "win32" ? ".cmd" : ""}`
118+
const nodeGyp = process.platform === "win32" ? which.sync("node-gyp") : "node-gyp"
118119
const args = ["rebuild"]
119120
// headers of old Electron versions do not have a valid config.gypi file
120121
// and --force-process-config must be passed to node-gyp >= 8.4.0 to

‎packages/builder-util/src/util.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,11 +121,14 @@ export function exec(file: string, args?: Array<string> | null, options?: ExecFi
121121
}
122122
resolve(stdout.toString())
123123
} else {
124+
const code = (error as any).code
124125
// https://github.com/npm/npm/issues/17624
125-
if ((file === "npm" || file === "npm.cmd") && args?.includes("list") && args?.includes("--silent")) {
126+
if ((file.toLowerCase().endsWith("npm") || file.toLowerCase().endsWith("npm.cmd")) && args?.includes("list") && args?.includes("--silent")) {
127+
console.error({ file, code }, error.message)
126128
resolve(stdout.toString())
129+
return
127130
}
128-
let message = chalk.red(removePassword(`Exit code: ${(error as any).code}. ${error.message}`))
131+
let message = chalk.red(removePassword(`Exit code: ${code}. ${error.message}`))
129132
if (stdout.length !== 0) {
130133
if (file.endsWith("wine")) {
131134
stdout = stdout.toString()

0 commit comments

Comments
 (0)