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

kozlove-aws · 2020-08-25T00:13:57Z

Problem

JSII is missing functionality for packing and using bin scripts from Python package.

Solution

Were extended assembly and project-info properties for bin-scripts.
Added generation scripts for each script in bin section.
Added section scripts to pyhon.py script.

Testing

Was build project with changed scripts and checked that file .jsii contains bin section.
Also was checked that without bin section in package.json empty bin section was not created in .jsii file.
Was created package and verified that scripts were created and paths to it were added to setup.py.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

RomainMuller · 2020-08-25T13:18:26Z

packages/@jsii/spec/lib/assembly.ts

+   *
+   * @default none
+   */
+  bin?: { [script: string]: string };


Let's tighten this one up from the start (cannot fix the older fields to readonly because... backwards-compatibility... but we can be nice with the new ones!)

Suggested change

bin?: { [script: string]: string };

bin?: { readonly [script: string]: string };

RomainMuller · 2020-08-25T13:19:07Z

packages/jsii-pacmak/lib/targets/python.ts

@@ -1429,6 +1429,13 @@ class PythonModule implements PythonType {
      code.line(`from ${'.'.repeat(distanceFromRoot + 1)}_jsii import *`);

      this.emitRequiredImports(code, context);
+      if (this.assembly.bin) {


I like being explicit here (JS' truthiness/falsiness is a little awkward)

Suggested change

if (this.assembly.bin) {

if (this.assembly.bin != null) {

RomainMuller · 2020-08-25T13:19:56Z

packages/jsii-pacmak/lib/targets/python.ts

+        const scripts = Object.keys(this.assembly.bin);
+        for (const script of scripts) {
+          code.line(`print('${script}: ${this.assembly.bin[script]}');`);
+        }


Suggested change

const scripts = Object.keys(this.assembly.bin);

for (const script of scripts) {

code.line(`print('${script}: ${this.assembly.bin[script]}');`);

}

const scripts = Object.keys(this.assembly.bin);

for (const [name, path] of Object.entries(this.assembly.bin)) {

code.line(`print('${name}: ${path}');`);

}

…ipt_folder

RomainMuller · 2020-10-07T08:18:50Z

packages/@jsii/kernel/lib/api.ts

@@ -103,13 +103,13 @@ export interface LoadResponse {
 export interface InvokeScriptRequest {
  readonly pkgname: string;
  readonly script: string;
-  readonly args?: any[];
+  readonly args?: string[];


Suggested change

readonly args?: string[];

readonly args?: readonly string[];

RomainMuller · 2020-10-07T08:27:47Z

packages/@jsii/kernel/lib/kernel.ts

+      'node',
+      [path.join(this._getPackageDir(req.pkgname), req.script)].concat(
+        req.args ?? [],
+      ),


It's probably going to be more efficient to use the same node runtime that is used by the Kernel process (and it's also going to be safer in Windows):

process.execPath contains the path to the node binary that runs the current process.

process.execArgv contains the node arguments that the current process received.

The -- indicates the end of node options and guarantees that none of your own command options are possibly interpreted by node itself.

Suggested change

'node',

[path.join(this._getPackageDir(req.pkgname), req.script)].concat(

req.args ?? [],

),

process.execPath,

[

...process.execArgv,

'--',

path.join(this._getPackageDir(req.pkgname), req.script),

...req.args ?? [],

],

RomainMuller · 2020-10-07T08:28:22Z

packages/@jsii/kernel/test/kernel.test.ts

+  sandbox.create({ fqn: 'jsii-calc.Calculator' });
+


I don't think this is necessary

Suggested change

sandbox.create({ fqn: 'jsii-calc.Calculator' });

RomainMuller · 2020-10-07T08:31:13Z

packages/@jsii/kernel/lib/api.ts

@@ -100,6 +100,19 @@ export interface LoadResponse {
  readonly types: number;
 }

+export interface InvokeScriptRequest {
+  readonly pkgname: string;


I would rather try to make sure we are consistent with how we refer to those concepts... in this case, we refer to assemblies (since the value that ought to be passed here is the same that was passed to LoadRequest.assembly).

Suggested change

readonly pkgname: string;

readonly assembly: string;

RomainMuller · 2020-10-07T08:36:31Z

packages/@jsii/kernel/lib/kernel.ts

+  ): api.InvokeScriptResponse {
+    const result = cp.spawnSync(
+      'node',
+      [path.join(this._getPackageDir(req.pkgname), req.script)].concat(


I think the script value provided should not be a path, instead it should be a key from the bin map that was gathered from package.json during compilation.

You need to validate the script name was in the map, and fail if not (you don't want to allow a request to run anything from anywhere, that's kind of a bad security posture to have. Then, you need to use the value that corresponds to the bin key that was requested in order to compose the final executable path.

Maybe we shouldn't actually use node shell out here... Your NPM package's bin script might not actually be a node script (could be shell script, could be something else). On the other hand it might be tricky to make this work smooth on Windows if we skip the shell out (I guess it's a good first step?)

RomainMuller · 2020-10-07T08:48:32Z

packages/@jsii/kernel/lib/kernel.ts

+      output: result.output,
+      stdout: result.stdout,
+      stderr: result.stderr,
+      status: result.status,


This might be null if the app was killed by a signal... Might want to also pass signal though.
The contract for spawnSync guarantees that exactly one of result.status and result.signal will be non-null.

Following my suggestion for the type change above... This would be something like:

Suggested change

status: result.status,

status: result.status != null

? { code: result.status }

: { signal: result.signal! }, // <- The `!` is to signal we "know" `result.signal` cannot be `null` here

I do not think that it is a good idea to add as result one of property. It could be really inconvenient to parse such property.
What do you think if we add signal as additional property?

RomainMuller · 2020-10-07T08:50:03Z

packages/jsii/lib/assembler.ts

      fingerprint: '<TBD>',
+      bin: this.projectInfo.bin,


Let's try to keep fingerprint last here, at least for now.

Suggested change

fingerprint: '<TBD>',

bin: this.projectInfo.bin,

bin: this.projectInfo.bin,

fingerprint: '<TBD>',

RomainMuller · 2020-10-07T08:51:25Z

packages/jsii-pacmak/lib/targets/python.ts

+        if (scripts.indexOf(script) < 0) {
+          scripts.push(script);
+        }


I think the duplication defense here is not necessary (it also does not hurt!)

It does not hurt but looks strange when the same script appeared several times.

But... it shouldn't... I mean a script can only be registered once... so if we have duplicates, it could mean something wrong happened somewhere else?

You are right. I have made some tests and have not found any dublicates.

RomainMuller · 2020-10-07T08:52:24Z

packages/jsii-pacmak/lib/targets/python.ts

@@ -1631,6 +1681,8 @@ class Package {
      a.pythonName.localeCompare(b.pythonName),
    );

+    const scripts: string[] = [];


We tend to prefer this form for some reason:

Suggested change

const scripts: string[] = [];

const scripts = new Array<string>();

I just to try keep style of this file. Such structure appear 5 times in this script.

RomainMuller · 2020-10-07T08:54:23Z

packages/jsii-pacmak/lib/targets/python.ts

+            '__jsii_assembly__ = jsii.JSIIAssembly.load(',
+            [
+              JSON.stringify(this.assembly.name),
+              JSON.stringify(this.assembly.version),
+              JSON.stringify(this.pythonName.replace('._jsii', '')),
+              `${JSON.stringify(this.assemblyFilename)}`,
+            ],


You will probably run into issues doing that, because this flow never loads the current package's dependencies into the kernel...

Instead of re-doing this, you should import the module that actually initializes things correctly:

import ._jsii # <- And voilà, dependencies and the current module are loaded!

Do not really understand what do you mean here?

RomainMuller · 2020-11-05T08:58:35Z

So sorry - this was closed in error as the master branch was renamed to main. Apologies!

RomainMuller · 2020-11-05T13:49:23Z

packages/@jsii/kernel/lib/kernel.ts

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


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,

},

);

RomainMuller · 2020-11-05T13:50:29Z

packages/@jsii/kernel/test/kernel.test.ts

+    script: 'calc',
+  });
+
+  expect(result.stdout).toEqual('Hello World!\n');


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

RomainMuller · 2020-11-05T13:50:46Z

packages/@jsii/python-runtime/tests/test_compliance.py

@@ -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


That doesn't appear to be necessary.

RomainMuller · 2020-11-05T13:52:30Z

packages/jsii-pacmak/lib/targets/python.ts

-          scripts.push(script);
-        }
-      }
+      Array.prototype.push.apply(scripts, mod.emitBinScripts(code));


Why not scripts.push(...mod.emitBinScripts(code))?

I just not very familiar with typescript so had no idea about '...' operator. Your version is better.

RomainMuller · 2020-11-05T13:54:27Z

packages/jsii-calc/test/assembly.jsii

@@ -7,6 +7,9 @@
    ],
    "url": "https://aws.amazon.com"
  },
+  "bin": {
+    "calc": "bin/calc"


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).

RomainMuller · 2020-12-10T10:42:49Z

@Mergifyio update

mergify · 2020-12-10T10:43:05Z

Command update: success

Branch has been successfully updated

mergify · 2020-12-10T10:43:07Z

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

aws-cdk-automation · 2020-12-10T11:03:27Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-5lHf64IXfvmr
Commit ID: fc2ec7c
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-12-10T11:56:32Z

Merging (with squash)...

feat: add bin script folder

b1547a2

RomainMuller reviewed Aug 25, 2020

View reviewed changes

RomainMuller changed the title ~~feat: Add bin script folder~~ feat: add support for bin-scripts (python only) Aug 25, 2020

kozlove-aws force-pushed the add_bin_script_folder branch from 10723f3 to c49762b Compare August 25, 2020 22:02

feat: Add creation scripts in bin folder and adding it to setup.py

aaf8d7e

kozlove-aws force-pushed the add_bin_script_folder branch from c49762b to aaf8d7e Compare August 25, 2020 22:04

SomayaB assigned MrArnoldPalmer Aug 31, 2020

kozlove-aws force-pushed the add_bin_script_folder branch 3 times, most recently from 1d7b7dc to 628ac54 Compare September 10, 2020 23:56

feat: add invokeBinScript function

7e9c9e2

kozlove-aws force-pushed the add_bin_script_folder branch from 628ac54 to 7e9c9e2 Compare September 11, 2020 19:48

RomainMuller and others added 3 commits September 24, 2020 16:48

Merge remote-tracking branch 'origin/master' into add_bin_script_folder

a1a8847

Add missing _ prefix to Kernel prototype private method

66943e6

Merge branch 'master' of https://github.com/aws/jsii into add_bin_scr…

48ad9f5

…ipt_folder

kozlove-aws force-pushed the add_bin_script_folder branch 8 times, most recently from af60019 to b0c6f98 Compare October 6, 2020 01:27

RomainMuller suggested changes Oct 7, 2020

View reviewed changes

kozlove-aws force-pushed the add_bin_script_folder branch from b0c6f98 to ed032d9 Compare October 9, 2020 02:13

feat: add unit test

dcfe863

kozlove-aws force-pushed the add_bin_script_folder branch from ed032d9 to dcfe863 Compare October 14, 2020 16:17

Merge branch 'master' into add_bin_script_folder

d5edeb2

RomainMuller closed this Oct 28, 2020

RomainMuller deleted the branch aws:main October 28, 2020 16:31

RomainMuller reopened this Nov 5, 2020

RomainMuller changed the base branch from master to main November 5, 2020 08:58

RomainMuller reviewed Nov 5, 2020

View reviewed changes

root and others added 9 commits November 6, 2020 00:33

fix: rename test script, small fixes

191650e

fix snapshots

375147b

Merge remote-tracking branch 'origin/main' into add_bin_script_folder

7ccdd96

Merge remote-tracking branch 'origin/main' into add_bin_script_folder

db66ffd

fix linter offense

122b17e

Add cmd wrapper for Windows

26b3a5b

Merge branch 'main' into add_bin_script_folder

2735e68

fix test expectations

1d7ced4

Fix Windows tests

f42d160

kozlove-aws marked this pull request as ready for review December 9, 2020 16:34

RomainMuller approved these changes Dec 10, 2020

View reviewed changes

Merge branch 'main' into add_bin_script_folder

fc2ec7c

mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Dec 10, 2020

mergify bot merged commit 61ef5ed into aws:main Dec 10, 2020

mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kozlove-aws commented Aug 25, 2020 •

edited

Loading

RomainMuller Aug 25, 2020

RomainMuller Aug 25, 2020

RomainMuller Aug 25, 2020

RomainMuller Oct 7, 2020

RomainMuller Oct 7, 2020

RomainMuller Oct 7, 2020

RomainMuller Oct 7, 2020

RomainMuller Oct 7, 2020

RomainMuller Oct 7, 2020

kozlove-aws Oct 8, 2020

RomainMuller Oct 7, 2020

RomainMuller Oct 7, 2020

kozlove-aws Oct 7, 2020

RomainMuller Oct 8, 2020

kozlove-aws Oct 9, 2020

RomainMuller Oct 7, 2020

kozlove-aws Oct 7, 2020

RomainMuller Oct 7, 2020

kozlove-aws Oct 9, 2020

RomainMuller commented Nov 5, 2020

RomainMuller Nov 5, 2020

RomainMuller Nov 5, 2020

RomainMuller Nov 5, 2020

RomainMuller Nov 5, 2020

kozlove-aws Nov 6, 2020 •

edited

Loading

RomainMuller Nov 5, 2020

kozlove-aws Nov 6, 2020

RomainMuller commented Dec 10, 2020

mergify bot commented Dec 10, 2020

mergify bot commented Dec 10, 2020

aws-cdk-automation commented Dec 10, 2020

mergify bot commented Dec 10, 2020

	bin?: { [script: string]: string };
	bin?: { readonly [script: string]: string };

-      status: result.status,
+      status: result.status != null
+        ? { code: result.status }
+        : { signal: result.signal! }, // <- The `!` is to signal we "know" `result.signal` cannot be `null` here

	const scripts: string[] = [];
	const scripts = new Array<string>();

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

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

Conversation

kozlove-aws commented Aug 25, 2020 • edited Loading

Problem

Solution

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomainMuller commented Nov 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlove-aws Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomainMuller commented Dec 10, 2020

mergify bot commented Dec 10, 2020

mergify bot commented Dec 10, 2020

aws-cdk-automation commented Dec 10, 2020

AWS CodeBuild CI Report

mergify bot commented Dec 10, 2020

kozlove-aws commented Aug 25, 2020 •

edited

Loading

kozlove-aws Nov 6, 2020 •

edited

Loading