-
Notifications
You must be signed in to change notification settings - Fork 246
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
feat(python): streaming bin script output #3794
Changes from all commits
77d3f14
2a5b889
9c6d1e4
c16927a
b205413
3bb11f8
7be5e65
f1a48de
089598f
efb47bb
75ca696
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 |
---|---|---|
|
@@ -169,50 +169,29 @@ export class Kernel { | |
}; | ||
} | ||
|
||
public getBinScriptCommand( | ||
req: api.GetScriptCommandRequest, | ||
): api.GetScriptCommandResponse { | ||
return this._getBinScriptCommand(req); | ||
} | ||
|
||
public invokeBinScript( | ||
req: api.InvokeScriptRequest, | ||
): api.InvokeScriptResponse { | ||
const packageDir = this._getPackageDir(req.assembly); | ||
if (fs.pathExistsSync(packageDir)) { | ||
// module exists, verify version | ||
const epkg = fs.readJsonSync(path.join(packageDir, 'package.json')); | ||
|
||
if (!epkg.bin) { | ||
throw new JsiiFault( | ||
'There is no bin scripts defined for this package.', | ||
); | ||
} | ||
|
||
const scriptPath = epkg.bin[req.script]; | ||
const { command, args, env } = this._getBinScriptCommand(req); | ||
|
||
if (!epkg.bin) { | ||
throw new JsiiFault(`Script with name ${req.script} was not defined.`); | ||
} | ||
|
||
const result = cp.spawnSync( | ||
path.join(packageDir, scriptPath), | ||
req.args ?? [], | ||
{ | ||
encoding: 'utf-8', | ||
env: { | ||
...process.env, | ||
// Make sure the current NODE_OPTIONS are honored if we shell out to node | ||
NODE_OPTIONS: process.execArgv.join(' '), | ||
// Make sure "this" node is ahead of $PATH just in case | ||
PATH: `${path.dirname(process.execPath)}:${process.env.PATH}`, | ||
}, | ||
shell: true, | ||
}, | ||
); | ||
const result = cp.spawnSync(command, args, { | ||
encoding: 'utf-8', | ||
env, | ||
shell: true, | ||
}); | ||
|
||
return { | ||
stdout: result.stdout, | ||
stderr: result.stderr, | ||
status: result.status, | ||
signal: result.signal, | ||
}; | ||
} | ||
throw new JsiiFault(`Package with name ${req.assembly} was not loaded.`); | ||
return { | ||
stdout: result.stdout, | ||
stderr: result.stderr, | ||
status: result.status, | ||
signal: result.signal, | ||
}; | ||
} | ||
|
||
public create(req: api.CreateRequest): api.CreateResponse { | ||
|
@@ -1325,6 +1304,38 @@ export class Kernel { | |
return property; | ||
} | ||
|
||
/** | ||
* Shared (non-public implementation) to as not to break API recording. | ||
*/ | ||
private _getBinScriptCommand( | ||
req: api.GetScriptCommandRequest, | ||
): api.GetScriptCommandResponse { | ||
const packageDir = this._getPackageDir(req.assembly); | ||
if (fs.pathExistsSync(packageDir)) { | ||
// module exists, verify version | ||
const epkg = fs.readJsonSync(path.join(packageDir, 'package.json')); | ||
|
||
const scriptPath = epkg.bin?.[req.script]; | ||
|
||
if (!epkg.bin) { | ||
throw new JsiiFault(`Script with name ${req.script} was not defined.`); | ||
} | ||
|
||
return { | ||
command: path.join(packageDir, scriptPath), | ||
args: req.args ?? [], | ||
env: { | ||
...process.env, | ||
// Make sure the current NODE_OPTIONS are honored if we shell out to node | ||
NODE_OPTIONS: process.execArgv.join(' '), | ||
Comment on lines
+1329
to
+1330
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. Not everything in 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. Same comment as above, this is a direct port of the existing I think, in this case, the contents of |
||
// Make sure "this" node is ahead of $PATH just in case | ||
PATH: `${path.dirname(process.execPath)}:${process.env.PATH}`, | ||
}, | ||
}; | ||
} | ||
throw new JsiiFault(`Package with name ${req.assembly} was not loaded.`); | ||
} | ||
|
||
// | ||
// type information | ||
// | ||
|
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.
On Windows, this may only work if the caller spawns that commend via a
shell
, which is a relatively easy way to inadvertently make your code not work on Windows...There would be a
.cmd
file that might be preferable to use as an entry point when on Windows if present...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.
I don't disagree. I was taking a "do no harm" approach with this PR by just carrying forward the existing logic, which would also not work on Windows.
I don't have easy access to a Windows host to test and understand the auto-generated
.cmd
file mentioned here: https://docs.npmjs.com/cli/v9/configuring-npm/package-json#binIs it essential that Windows support be included in the PR, since this isn't a new regression?
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.
Well... no. I have this principle that I never demand contributors leave the place cleaner than they found it 😅