-
Notifications
You must be signed in to change notification settings - Fork 52
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
fix(dotnet): should work with paths that contain spaces #392
Changes from 6 commits
5df3a4a
cb9a4c7
e545b4b
b7b7da5
a3bae30
f3ddb86
c818c77
6e28a15
4ee665d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
import { ChildProcess, execSync, spawn } from 'child_process'; | ||
|
||
import { getParameterString, swapKeysUsingMap } from '@nx-dotnet/utils'; | ||
import { getParameterArrayStrings, swapKeysUsingMap } from '@nx-dotnet/utils'; | ||
import { ChildProcess, spawn, spawnSync } from 'child_process'; | ||
|
||
import { | ||
addPackageKeyMap, | ||
|
@@ -24,160 +23,161 @@ import { LoadedCLI } from './dotnet.factory'; | |
export class DotNetClient { | ||
constructor(private cliCommand: LoadedCLI, public cwd?: string) {} | ||
|
||
new(template: dotnetTemplate, parameters?: dotnetNewOptions): void { | ||
let cmd = `${this.cliCommand.command} new ${template}`; | ||
new(template: dotnetTemplate, parameters?: dotnetNewOptions): Buffer { | ||
const params = [`new`, template]; | ||
if (parameters) { | ||
parameters = swapKeysUsingMap(parameters, newKeyMap); | ||
const paramString = parameters ? getParameterString(parameters) : ''; | ||
cmd = `${cmd} ${paramString}`; | ||
params.push(...getParameterArrayStrings(parameters)); | ||
} | ||
return this.logAndExecute(cmd); | ||
return this.logAndExecute(params); | ||
} | ||
|
||
build(project: string, parameters?: dotnetBuildOptions): void { | ||
let cmd = `${this.cliCommand.command} build "${project}"`; | ||
build(project: string, parameters?: dotnetBuildOptions): Buffer { | ||
const params = [`build`, project]; | ||
if (parameters) { | ||
parameters = swapKeysUsingMap(parameters, buildKeyMap); | ||
const paramString = parameters ? getParameterString(parameters) : ''; | ||
cmd = `${cmd} ${paramString}`; | ||
params.push(...getParameterArrayStrings(parameters)); | ||
} | ||
return this.logAndExecute(cmd); | ||
return this.logAndExecute(params); | ||
} | ||
|
||
run( | ||
project: string, | ||
watch = false, | ||
parameters?: dotnetRunOptions, | ||
): ChildProcess { | ||
let cmd = watch | ||
? `watch --project "${project}" run` | ||
: `run --project "${project}"`; | ||
const params = watch | ||
? [`watch`, `--project`, project, `run`] | ||
: [`run`, `--project`, project]; | ||
if (parameters) { | ||
parameters = swapKeysUsingMap(parameters, runKeyMap); | ||
const paramString = parameters ? getParameterString(parameters) : ''; | ||
cmd = `${cmd} ${paramString}`; | ||
params.push(...getParameterArrayStrings(parameters)); | ||
} | ||
console.log(`Executing Command: dotnet ${cmd}`); | ||
return spawn(this.cliCommand.command, cmd.split(' '), { | ||
stdio: 'inherit', | ||
cwd: this.cwd, | ||
}); | ||
|
||
return this.logAndSpawn(params); | ||
} | ||
|
||
test( | ||
project: string, | ||
watch?: boolean, | ||
parameters?: dotnetTestOptions, | ||
): void | ChildProcess { | ||
let cmd = watch ? `watch --project "${project}" test` : `test "${project}"`; | ||
cmd = `${this.cliCommand.command} ${cmd}`; | ||
): Buffer | ChildProcess { | ||
const params = watch | ||
? [`watch`, `--project`, project, `test`] | ||
: [`test`, project]; | ||
|
||
if (parameters) { | ||
const mappedParameters = swapKeysUsingMap(parameters, testKeyMap); | ||
const paramString = getParameterString(mappedParameters); | ||
cmd = `${cmd} ${paramString}`; | ||
parameters = swapKeysUsingMap(parameters, testKeyMap); | ||
params.push(...getParameterArrayStrings(parameters)); | ||
} | ||
if (!watch) { | ||
return this.logAndExecute(cmd); | ||
return this.logAndExecute(params); | ||
} else { | ||
console.log(`Executing Command: ${cmd}`); | ||
const params = cmd | ||
.split(' ') | ||
.slice(1) | ||
.filter((x) => x.length); | ||
return spawn(this.cliCommand.command, params, { | ||
stdio: 'inherit', | ||
cwd: this.cwd, | ||
}); | ||
const slicedParams = params.slice(1).filter((x) => x.length); | ||
return this.logAndSpawn(slicedParams); | ||
} | ||
} | ||
|
||
addPackageReference( | ||
project: string, | ||
pkg: string, | ||
parameters?: dotnetAddPackageOptions, | ||
): void { | ||
let cmd = `${this.cliCommand.command} add "${project}" package ${pkg}`; | ||
): Buffer { | ||
const params = [`add`, project, `package`, pkg]; | ||
if (parameters) { | ||
parameters = swapKeysUsingMap(parameters, addPackageKeyMap); | ||
const paramString = parameters ? getParameterString(parameters) : ''; | ||
cmd = `${cmd} ${paramString}`; | ||
params.push(...getParameterArrayStrings(parameters)); | ||
} | ||
return this.logAndExecute(cmd); | ||
return this.logAndExecute(params); | ||
} | ||
|
||
addProjectReference(hostCsProj: string, targetCsProj: string): void { | ||
return this.logAndExecute( | ||
`${this.cliCommand.command} add ${hostCsProj} reference ${targetCsProj}`, | ||
); | ||
addProjectReference(hostCsProj: string, targetCsProj: string): Buffer { | ||
return this.logAndExecute([`add`, hostCsProj, `reference`, targetCsProj]); | ||
} | ||
|
||
publish( | ||
project: string, | ||
parameters?: dotnetPublishOptions, | ||
publishProfile?: string, | ||
extraParameters?: string, | ||
): void { | ||
let cmd = `${this.cliCommand.command} publish "${project}"`; | ||
): Buffer { | ||
const params = [`publish`, `"${project}"`]; | ||
if (parameters) { | ||
parameters = swapKeysUsingMap(parameters, publishKeyMap); | ||
const paramString = parameters ? getParameterString(parameters) : ''; | ||
cmd = `${cmd} ${paramString}`; | ||
params.push(...getParameterArrayStrings(parameters)); | ||
} | ||
if (publishProfile) { | ||
cmd = `${cmd} -p:PublishProfile=${publishProfile}`; | ||
params.push(`-p:PublishProfile=${publishProfile}`); | ||
} | ||
if (extraParameters) { | ||
cmd = `${cmd} ${extraParameters}`; | ||
params.push(`${extraParameters}`); | ||
} | ||
return this.logAndExecute(cmd); | ||
return this.logAndExecute(params); | ||
} | ||
|
||
installTool(tool: string): void { | ||
const cmd = `${this.cliCommand.command} tool install ${tool}`; | ||
installTool(tool: string): Buffer { | ||
const cmd = [`tool`, `install`, tool]; | ||
return this.logAndExecute(cmd); | ||
} | ||
|
||
restorePackages(project: string): void { | ||
const cmd = `${this.cliCommand.command} restore "${project}"`; | ||
restorePackages(project: string): Buffer { | ||
const cmd = [`restore`, project]; | ||
return this.logAndExecute(cmd); | ||
} | ||
|
||
restoreTools(): void { | ||
const cmd = `${this.cliCommand.command} tool restore`; | ||
restoreTools(): Buffer { | ||
const cmd = [`tool`, `restore`]; | ||
return this.logAndExecute(cmd); | ||
} | ||
|
||
format(project: string, parameters?: dotnetFormatOptions): void { | ||
let cmd = `${this.cliCommand.command} format "${project}"`; | ||
format(project: string, parameters?: dotnetFormatOptions): Buffer { | ||
const params = [`format`, project]; | ||
if (parameters) { | ||
parameters = swapKeysUsingMap(parameters, formatKeyMap); | ||
const paramString = parameters ? getParameterString(parameters) : ''; | ||
cmd = `${cmd} ${paramString}`; | ||
params.push(...getParameterArrayStrings(parameters)); | ||
} | ||
return this.logAndExecute(cmd); | ||
return this.logAndExecute(params); | ||
} | ||
|
||
addProjectToSolution(solutionFile: string, project: string) { | ||
const cmd = `${this.cliCommand.command} sln "${solutionFile}" add "${project}"`; | ||
this.logAndExecute(cmd); | ||
const params = [`sln`, solutionFile, `add`, project]; | ||
this.logAndExecute(params); | ||
} | ||
|
||
getSdkVersion(): Buffer { | ||
const cmd = 'dotnet --version'; | ||
return this.execute(cmd); | ||
return this.execute(['--version']); | ||
} | ||
|
||
printSdkVersion(): void { | ||
this.logAndExecute('dotnet --version'); | ||
this.logAndExecute(['--version']); | ||
} | ||
|
||
private logAndExecute(cmd: string): void { | ||
console.log(`Executing Command: ${cmd}`); | ||
execSync(cmd, { stdio: 'inherit', cwd: this.cwd || process.cwd() }); | ||
private logAndExecute(params: string[]): Buffer { | ||
console.log( | ||
`Executing Command: ${this.cliCommand.command} ${params.join(', ')}`, | ||
); | ||
return this.execute(params); | ||
} | ||
|
||
private execute(cmd: string): Buffer { | ||
return execSync(cmd, { cwd: this.cwd || process.cwd() }); | ||
private execute(params: string[]): Buffer { | ||
return spawnSync(this.cliCommand.command, params, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we swapping to spawnSync over execSync here? This is more complex maintainability-wise and shouldn't be needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not totally opposed to spawnSync actually, some stuff does work out better this way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, had a lengthy meeting couldn't look into this. I converted to spawnSync to avoid converting all parameters to string first, spawnSync let us pass each parameter on the array so it is automatically escaped. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So with nx-plugins and other dev tooling in general, the hotspots that pop up from sonarqube can be mostly ignored. The reason being that those commands are being run by the developer, on their own machine. It's not really an attack point, as someone would already have to be able to run arbitrary commands to exercise it. The solution for most things that come up like that, are just for me to hit the "not a problem" button while reviewing the PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be a lot easier because I had to change the method a lot, I'll take it into account next time, for this one I was kinda in a rush to have a deployable version because the previous one was broken. |
||
stdio: 'inherit', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding 'inherit' here causes the returned buffers to be empty There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a new solution for this have to discuss with you, I didn't merge it yet. This should resolve the problem: private execute(params: string[], stdio: StdioOptions = 'pipe'): Buffer {
const proc = spawnSync(this.cliCommand.command, params, {
stdio,
cwd: this.cwd || process.cwd(),
});
let result = Buffer.alloc(0);
if (proc?.stdout) {
result = Buffer.concat([result, proc.stdout]);
}
if (proc?.stderr) {
result = Buffer.concat([result, proc.stderr]);
}
return result;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would work, but its fine to duplicate the spawnSync call for this. The idea behind There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with different calls for spawnSync with different behavior, it's easier to not mess it up. |
||
cwd: this.cwd || process.cwd(), | ||
}) | ||
.output.filter((buf) => buf !== null) | ||
.reduce( | ||
(acc, buf) => Buffer.concat([acc as Buffer, buf as Buffer]), | ||
Buffer.from(''), | ||
) as Buffer; | ||
} | ||
|
||
private logAndSpawn(params: string[]): ChildProcess { | ||
console.log( | ||
`Executing Command: ${this.cliCommand.command} ${params.join(', ')}`, | ||
); | ||
return spawn(this.cliCommand.command, params, { | ||
stdio: 'inherit', | ||
cwd: this.cwd || process.cwd(), | ||
}); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method needs the 'stdio: inherit' so that things like
nx build
show terminal output.