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

feat: add support for bin-scripts (python only) #1941

Merged
merged 18 commits into from
Dec 10, 2020
Merged
Show file tree
Hide file tree
Changes from 7 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
13 changes: 13 additions & 0 deletions packages/@jsii/kernel/lib/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,19 @@ export interface LoadResponse {
readonly types: number;
}

export interface InvokeScriptRequest {
readonly assembly: string;
readonly script: string;
readonly args?: string[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonly args?: string[];
readonly args?: readonly string[];

}

export interface InvokeScriptResponse {
readonly status: number | null;
readonly stdout: Buffer;
readonly stderr: Buffer;
readonly signal: string | null;
}

export interface CreateRequest {
/**
* The FQN of the class of which an instance is requested (or "Object")
Expand Down
71 changes: 57 additions & 14 deletions packages/@jsii/kernel/lib/kernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import * as api from './api';
import { TOKEN_REF } from './api';
import { ObjectTable, tagJsiiConstructor } from './objects';
import * as wire from './serialization';
import * as cp from 'child_process';

export class Kernel {
/**
Expand Down Expand Up @@ -66,24 +67,11 @@ export class Kernel {
);
}

if (!this.installDir) {
this.installDir = fs.mkdtempSync(path.join(os.tmpdir(), 'jsii-kernel-'));
fs.mkdirpSync(path.join(this.installDir, 'node_modules'));
this._debug('creating jsii-kernel modules workdir:', this.installDir);

process.on('exit', () => {
if (this.installDir) {
this._debug('removing install dir', this.installDir);
fs.removeSync(this.installDir); // can't use async version during exit
}
});
}

const pkgname = req.name;
const pkgver = req.version;

// check if we already have such a module
const packageDir = path.join(this.installDir, 'node_modules', pkgname);
const packageDir = this._getPackageDir(pkgname);
if (fs.pathExistsSync(packageDir)) {
// module exists, verify version
const epkg = fs.readJsonSync(path.join(packageDir, 'package.json'));
Expand Down Expand Up @@ -148,6 +136,45 @@ export class Kernel {
};
}

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 Error('There is no bin scripts defined for this package.');
}

const scriptPath = epkg.bin[req.script];

if (!epkg.bin) {
throw new Error(`Script with name ${req.script} was not defined.`);
}

const result = cp.spawnSync(
process.execPath,
[
...process.execArgv,
'--',
path.join(packageDir, scriptPath),
...(req.args ?? []),
],
{ encoding: 'utf-8' },
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script might not be a node script (could be bash, etc...), so this feels a little unsafe.

Suggested change
const result = cp.spawnSync(
process.execPath,
[
...process.execArgv,
'--',
path.join(packageDir, scriptPath),
...(req.args ?? []),
],
{ encoding: 'utf-8' },
);
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,
},
);


return {
stdout: result.stdout,
stderr: result.stderr,
status: result.status,
signal: result.signal,
};
}
throw new Error(`Package with name ${req.assembly} was not loaded.`);
}

public create(req: api.CreateRequest): api.CreateResponse {
return this._create(req);
}
Expand Down Expand Up @@ -494,6 +521,22 @@ export class Kernel {
}
}

private _getPackageDir(pkgname: string): string {
if (!this.installDir) {
this.installDir = fs.mkdtempSync(path.join(os.tmpdir(), 'jsii-kernel-'));
fs.mkdirpSync(path.join(this.installDir, 'node_modules'));
this._debug('creating jsii-kernel modules workdir:', this.installDir);

process.on('exit', () => {
if (this.installDir) {
this._debug('removing install dir', this.installDir);
fs.removeSync(this.installDir); // can't use async version during exit
}
});
}
return path.join(this.installDir, 'node_modules', pkgname);
}

// prefixed with _ to allow calling this method internally without
// getting it recorded for testing.
private _create(req: api.CreateRequest): api.CreateResponse {
Expand Down
9 changes: 9 additions & 0 deletions packages/@jsii/kernel/test/kernel.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2133,6 +2133,15 @@ defineTest('Override transitive property', (sandbox) => {
expect(propValue).toBe('N3W');
});

defineTest('invokeBinScript() return output', (sandbox) => {
const result = sandbox.invokeBinScript({
assembly: 'jsii-calc',
script: 'calc',
});

expect(result.stdout).toEqual('Hello World!\n');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably should also asset the value of stderr, status and signal.

});

// =================================================================================================

const testNames: { [name: string]: boolean } = {};
Expand Down
15 changes: 6 additions & 9 deletions packages/@jsii/kernel/test/recording.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,11 @@ export function recordInteraction(kernel: Kernel, inputOutputLogPath: string) {
const logfile = fs.createWriteStream(inputOutputLogPath);
(kernel as any).logfile = logfile;

Object.getOwnPropertyNames(Kernel.prototype)
.filter((p) => !p.startsWith('_'))
.forEach((api) => {
const old = Object.getOwnPropertyDescriptor(Kernel.prototype, api)!;

Object.entries(Object.getOwnPropertyDescriptors(Kernel.prototype))
.filter(([p, v]) => !p.startsWith('_') && typeof v.value === 'function')
.forEach(([api, old]) => {
Object.defineProperty(kernel, api, {
...old,
value(...args: any[]) {
logInput({ api, ...args[0] });
try {
Expand Down Expand Up @@ -67,12 +66,10 @@ export function recordInteraction(kernel: Kernel, inputOutputLogPath: string) {
});

function logInput(obj: any) {
const inputLine = `${JSON.stringify(obj)}\n`;
logfile.write(`> ${inputLine}`);
logfile.write(`> ${JSON.stringify(obj)}\n`);
}

function logOutput(obj: any) {
const outputLine = `${JSON.stringify(obj)}\n`;
logfile.write(`< ${outputLine}`);
logfile.write(`< ${JSON.stringify(obj)}\n`);
}
}
15 changes: 15 additions & 0 deletions packages/@jsii/python-runtime/src/jsii/_kernel/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
GetResponse,
InvokeRequest,
InvokeResponse,
InvokeScriptRequest,
KernelResponse,
LoadRequest,
ObjRef,
Expand Down Expand Up @@ -242,6 +243,20 @@ def __init__(self, provider_class: Type[BaseProvider] = ProcessProvider) -> None
def load(self, name: str, version: str, tarball: str) -> None:
self.provider.load(LoadRequest(name=name, version=version, tarball=tarball))

def invokeBinScript(
self, pkgname: str, script: str, args: Optional[List[Any]] = None
) -> None:
if args is None:
args = []

self.provider.invokeBinScript(
InvokeScriptRequest(
pkgname=pkgname,
script=script,
args=_make_reference_for_native(self, args),
)
)

# TODO: Is there a way to say that obj has to be an instance of klass?
def create(self, klass: Type, obj: Any, args: Optional[List[Any]] = None) -> ObjRef:
if args is None:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
GetResponse,
InvokeRequest,
InvokeResponse,
InvokeScriptRequest,
InvokeScriptResponse,
DeleteRequest,
DeleteResponse,
SetRequest,
Expand Down Expand Up @@ -45,6 +47,10 @@ class BaseProvider(metaclass=abc.ABCMeta):
def load(self, request: LoadRequest) -> LoadResponse:
...

@abc.abstractmethod
def invokeBinScript(self, request: InvokeScriptRequest) -> InvokeScriptResponse:
...

@abc.abstractmethod
def create(self, request: CreateRequest) -> CreateResponse:
...
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
GetResponse,
InvokeRequest,
InvokeResponse,
InvokeScriptRequest,
InvokeScriptResponse,
SetRequest,
SetResponse,
StaticGetRequest,
Expand Down Expand Up @@ -157,6 +159,10 @@ def __init__(self):
LoadRequest,
_with_api_key("load", self._serializer.unstructure_attrs_asdict),
)
self._serializer.register_unstructure_hook(
InvokeScriptRequest,
_with_api_key("invokeBinScript", self._serializer.unstructure_attrs_asdict),
)
self._serializer.register_unstructure_hook(
CreateRequest,
_with_api_key("create", self._serializer.unstructure_attrs_asdict),
Expand Down Expand Up @@ -332,6 +338,9 @@ def _process(self) -> _NodeProcess:
def load(self, request: LoadRequest) -> LoadResponse:
return self._process.send(request, LoadResponse)

def invokeBinScript(self, request: InvokeScriptRequest) -> InvokeScriptResponse:
return self._process.send(request, InvokeScriptResponse)

def create(self, request: CreateRequest) -> CreateResponse:
return self._process.send(request, CreateResponse)

Expand Down
19 changes: 19 additions & 0 deletions packages/@jsii/python-runtime/src/jsii/_kernel/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,23 @@ class LoadResponse:
types: int


@attr.s(auto_attribs=True, frozen=True, slots=True)
class InvokeScriptRequest:

pkgname: str
script: str
args: List[Any] = attr.Factory(list)


@attr.s(auto_attribs=True, frozen=True, slots=True)
class InvokeScriptResponse:

status: int
stdout: str
stderr: str
output: List[str]


@attr.s(auto_attribs=True, frozen=True, slots=True)
class CreateRequest:

Expand Down Expand Up @@ -226,6 +243,7 @@ class StatsResponse:
GetRequest,
StaticGetRequest,
InvokeRequest,
InvokeScriptRequest,
StaticInvokeRequest,
StatsRequest,
]
Expand All @@ -237,6 +255,7 @@ class StatsResponse:
DeleteResponse,
GetResponse,
InvokeResponse,
InvokeScriptResponse,
SetResponse,
StatsResponse,
Callback,
Expand Down
4 changes: 4 additions & 0 deletions packages/@jsii/python-runtime/src/jsii/_runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ def load(cls, *args, _kernel=kernel, **kwargs):
# Give our record of the assembly back to the caller.
return assembly

def invokeBinScript(cls, pkgname, script, *args, _kernel=kernel):
response = _kernel.invokeBinScript(pkgname, script, *args)
print(response.stdout)


class JSIIMeta(_ClassPropertyMeta, type):
def __new__(cls, name, bases, attrs, *, jsii_type=None):
Expand Down
1 change: 1 addition & 0 deletions packages/@jsii/python-runtime/tests/test_compliance.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
from scope.jsii_calc_lib import IFriendly, EnumFromScopedModule, Number
from scope.jsii_calc_lib.custom_submodule_name import IReflectable, ReflectableEntry

from subprocess import Popen, PIPE, STDOUT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't appear to be necessary.


# Note: The names of these test functions have been chosen to map as closely to the
# Java Compliance tests as possible.
Expand Down
7 changes: 7 additions & 0 deletions packages/@jsii/spec/lib/assembly.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,13 @@ export interface Assembly extends AssemblyConfiguration, Documentable {
* @default none
*/
readme?: { markdown: string };

/**
* List of bin-scripts
*
* @default none
*/
bin?: { readonly [script: string]: string };
}

/**
Expand Down
2 changes: 2 additions & 0 deletions packages/jsii-calc/bin/calc
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
#!/usr/bin/env node
require('./calc.js');
5 changes: 5 additions & 0 deletions packages/jsii-calc/bin/calc.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#!/usr/bin/env node

/* eslint-disable no-console */

console.info('Hello World!');
3 changes: 3 additions & 0 deletions packages/jsii-calc/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
"bugs": {
"url": "https://github.com/aws/jsii/issues"
},
"bin": {
"calc": "bin/calc"
},
"repository": {
"type": "git",
"url": "https://github.com/aws/jsii.git",
Expand Down
5 changes: 4 additions & 1 deletion packages/jsii-calc/test/assembly.jsii
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
],
"url": "https://aws.amazon.com"
},
"bin": {
"calc": "bin/calc"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be interesting to rename bin/calc to something else to remove the "coincidence" that it matches the script's name (the key here).

For example, you could call it bin/run instead.

There's also a chance this does not test fine on Windows (because Windows is hard 🤓). In such cases you'd need to drop in a CMD entry point (I'll give you more info on this once we know this is necessary, hopefully it won't be).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

},
"bundled": {
"@fixtures/jsii-calc-bundled": "^0.19.0"
},
Expand Down Expand Up @@ -14229,5 +14232,5 @@
}
},
"version": "0.0.0",
"fingerprint": "azqNkkl+/4FLzTVBLkOyHcokS4xLoYtHsri0z9kIehQ="
"fingerprint": "QHc8YZS13IljwCQpg6AZlBxIZvkAbfnCFh6Vi+e0Cgg="
}
Loading