From 097a007d8943007afdae34ffce3a6d99fc51707d Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Thu, 9 Jul 2020 17:57:49 +0100 Subject: [PATCH 1/2] fix(pacmak): python pack fails when installing 'black' via pip The failure is because of intermittent unavailability of black or one of its packages from the PyPI index. Instead, install back into the environment, i.e., superchain. Fallback to installing it once in the user's home directory. --- packages/jsii-pacmak/lib/targets/python.ts | 93 +++++++++++-------- packages/jsii-pacmak/lib/util.ts | 2 +- packages/jsii-pacmak/package.json | 2 +- .../jsii-pacmak/test/targets/python.test.ts | 51 ++++++++++ superchain/Dockerfile | 2 +- 5 files changed, 108 insertions(+), 42 deletions(-) create mode 100644 packages/jsii-pacmak/test/targets/python.test.ts diff --git a/packages/jsii-pacmak/lib/targets/python.ts b/packages/jsii-pacmak/lib/targets/python.ts index 328351d363..d20d8dac04 100644 --- a/packages/jsii-pacmak/lib/targets/python.ts +++ b/packages/jsii-pacmak/lib/targets/python.ts @@ -7,7 +7,7 @@ import * as path from 'path'; import * as spec from '@jsii/spec'; import { Stability } from '@jsii/spec'; import { Generator, GeneratorOptions } from '../generator'; -import { warn } from '../logging'; +import { info, warn } from '../logging'; import { md2rst } from '../markdown'; import { Target, TargetOptions } from '../target'; import { shell } from '../util'; @@ -34,6 +34,11 @@ import { die, toPythonIdentifier } from './python/util'; const spdxLicenseList = require('spdx-license-list'); export default class Python extends Target { + private static readonly BLACK_INSTALL_DIR: string = path.join( + os.homedir(), + 'python-black', + ); + protected readonly generator: PythonGenerator; public constructor(options: TargetOptions) { @@ -46,28 +51,9 @@ export default class Python extends Target { await super.generateCode(outDir, tarball); // We'll just run "black" on that now, to make the generated code a little more readable. - const blackRoot = await fs.mkdtemp( - path.join(os.tmpdir(), 'jsii-pacmak-black-'), - ); - try { - await shell('python3', ['-m', 'venv', path.join(blackRoot, '.env')], { - cwd: blackRoot, - }); - await shell( - path.join(blackRoot, '.env', 'bin', 'pip'), - ['install', 'black'], - { cwd: blackRoot }, - ); - await shell( - path.join(blackRoot, '.env', 'bin', 'black'), - ['--py36', outDir], - { - cwd: outDir, - }, - ); - } finally { - await fs.remove(blackRoot); - } + await shell(await this.blackPath(), ['--py36', outDir], { + cwd: outDir, + }); } public async build(sourceDir: string, outDir: string): Promise { @@ -78,7 +64,7 @@ export default class Python extends Target { await shell('python3', ['setup.py', 'bdist_wheel', '--dist-dir', outDir], { cwd: sourceDir, }); - if (await twineIsPresent()) { + if (await isPresent('twine', sourceDir)) { await shell('twine', ['check', path.join(outDir, '*')], { cwd: sourceDir, }); @@ -88,22 +74,51 @@ export default class Python extends Target { 'Run `pip3 install twine` to enable distribution package validation.', ); } + } - // Approximating existence check using `which`, falling back on `pip3 show`. If that fails, assume twine is not there. - async function twineIsPresent(): Promise { - try { - await shell('which', ['twine'], { cwd: sourceDir }); - return true; - } catch { - try { - const output = await shell('pip3', ['show', 'twine'], { - cwd: sourceDir, - }); - return output.trim() !== ''; - } catch { - return false; - } - } + private async blackPath(): Promise { + if (await isPresent('black')) { + return 'black'; + } + + const exists = await fs.pathExists(Python.BLACK_INSTALL_DIR); + if (!exists) { + info( + `No existing black installation. Install afresh at ${Python.BLACK_INSTALL_DIR}...`, + ); + await fs.mkdir(Python.BLACK_INSTALL_DIR); + await shell( + 'python3', + ['-m', 'venv', path.join(Python.BLACK_INSTALL_DIR, '.env')], + { + cwd: Python.BLACK_INSTALL_DIR, + }, + ); + await shell( + path.join(Python.BLACK_INSTALL_DIR, '.env', 'bin', 'pip'), + ['install', 'black'], + { cwd: Python.BLACK_INSTALL_DIR }, + ); + } + return path.join(Python.BLACK_INSTALL_DIR, '.env', 'bin', 'black'); + } +} + +// Approximating existence check using `which`, falling back on `pip3 show`. +async function isPresent(binary: string, sourceDir?: string): Promise { + try { + await shell('which', [binary], { + cwd: sourceDir, + }); + return true; + } catch { + try { + const output = await shell('pip3', ['show', binary], { + cwd: sourceDir, + }); + return output.trim() !== ''; + } catch { + return false; } } } diff --git a/packages/jsii-pacmak/lib/util.ts b/packages/jsii-pacmak/lib/util.ts index 41738b6d32..fdd6c596e1 100644 --- a/packages/jsii-pacmak/lib/util.ts +++ b/packages/jsii-pacmak/lib/util.ts @@ -33,7 +33,7 @@ export function resolveDependencyDirectory( export async function shell( cmd: string, args: string[], - options: ShellOptions, + options: ShellOptions = {}, ): Promise { // eslint-disable-next-line @typescript-eslint/require-await async function spawn1() { diff --git a/packages/jsii-pacmak/package.json b/packages/jsii-pacmak/package.json index e4305326f9..e597c05c2f 100644 --- a/packages/jsii-pacmak/package.json +++ b/packages/jsii-pacmak/package.json @@ -30,7 +30,7 @@ "watch": "tsc --build -w", "lint": "eslint . --ext .js,.ts --ignore-path=.gitignore", "lint:fix": "yarn lint --fix", - "test": "/bin/bash test/diff-test.sh && /bin/bash test/build-test.sh && jest", + "test": "jest && /bin/bash test/diff-test.sh && /bin/bash test/build-test.sh", "test:update": "UPDATE_DIFF=1 /bin/bash test/diff-test.sh && /bin/bash test/build-test.sh && jest -u", "package": "package-js" }, diff --git a/packages/jsii-pacmak/test/targets/python.test.ts b/packages/jsii-pacmak/test/targets/python.test.ts new file mode 100644 index 0000000000..ff700fb1c8 --- /dev/null +++ b/packages/jsii-pacmak/test/targets/python.test.ts @@ -0,0 +1,51 @@ +import * as util from '../../lib/util'; +import * as os from 'os'; +import * as path from 'path'; +import * as fs from 'fs-extra'; +import Python from '../../lib/targets/python'; +import { Assembly } from 'jsii-reflect'; +import { Rosetta } from 'jsii-rosetta'; + +describe('python', () => { + describe('blackPath', () => { + const shellMock = jest.fn(); + const osMock = jest.fn(); + let tmpdir: string; + + beforeEach(async () => { + // eslint-disable-next-line no-import-assign + Object.defineProperty(util, 'shell', { value: shellMock }); + // eslint-disable-next-line no-import-assign + Object.defineProperty(os, 'homedir', { value: osMock }); + tmpdir = await fs.mkdtemp(path.join(os.tmpdir(), 'jsii-pacmak-black-')); + osMock.mockImplementation(() => tmpdir); + }); + + afterEach(async () => { + shellMock.mockClear(); + osMock.mockClear(); + await fs.remove(tmpdir); + }); + + test('black is installed globally', async () => { + shellMock.mockImplementation((cmd: string, args: string[], _) => { + return new Promise((ok, ko) => { + if (cmd === 'which' && args[0] === 'black') { + ok('/path/to/black'); + } + ko(`Unexpected call to shell ${cmd} ${args}`); + }); + }); + + const python = new Python({ + targetName: 'python', + packageDir: '/dir', + assembly: {} as Assembly, + rosetta: new Rosetta(), + arguments: {}, + }); + const path = await (python as any).blackPath(); + expect(path).toBe('black'); + }); + }); +}); diff --git a/superchain/Dockerfile b/superchain/Dockerfile index b554a818c8..3067c72a86 100644 --- a/superchain/Dockerfile +++ b/superchain/Dockerfile @@ -20,7 +20,7 @@ RUN rpm --import "https://packages.microsoft.com/keys/microsoft.asc" # Install Python 3 RUN yum -y install python3 python3-pip \ - && python3 -m pip install --upgrade pip setuptools wheel twine \ + && python3 -m pip install --upgrade pip setuptools wheel twine black \ && yum clean all && rm -rf /var/cache/yum # Install Ruby 2.6+ From 557585d9f46a2461e342bb44d957eb219f8ef9df Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Fri, 10 Jul 2020 10:47:22 +0100 Subject: [PATCH 2/2] more tests --- packages/jsii-pacmak/lib/targets/python.ts | 26 +++--- .../jsii-pacmak/test/targets/python.test.ts | 89 +++++++++++++++---- 2 files changed, 86 insertions(+), 29 deletions(-) diff --git a/packages/jsii-pacmak/lib/targets/python.ts b/packages/jsii-pacmak/lib/targets/python.ts index d20d8dac04..448b7ab459 100644 --- a/packages/jsii-pacmak/lib/targets/python.ts +++ b/packages/jsii-pacmak/lib/targets/python.ts @@ -34,11 +34,6 @@ import { die, toPythonIdentifier } from './python/util'; const spdxLicenseList = require('spdx-license-list'); export default class Python extends Target { - private static readonly BLACK_INSTALL_DIR: string = path.join( - os.homedir(), - 'python-black', - ); - protected readonly generator: PythonGenerator; public constructor(options: TargetOptions) { @@ -81,26 +76,31 @@ export default class Python extends Target { return 'black'; } - const exists = await fs.pathExists(Python.BLACK_INSTALL_DIR); + const blackInstallDir = path.join( + os.homedir(), + '.jsii-cache', + 'python-black', + ); + const exists = await fs.pathExists(blackInstallDir); if (!exists) { info( - `No existing black installation. Install afresh at ${Python.BLACK_INSTALL_DIR}...`, + `No existing black installation. Install afresh at ${blackInstallDir}...`, ); - await fs.mkdir(Python.BLACK_INSTALL_DIR); + await fs.mkdirp(blackInstallDir); await shell( 'python3', - ['-m', 'venv', path.join(Python.BLACK_INSTALL_DIR, '.env')], + ['-m', 'venv', path.join(blackInstallDir, '.env')], { - cwd: Python.BLACK_INSTALL_DIR, + cwd: blackInstallDir, }, ); await shell( - path.join(Python.BLACK_INSTALL_DIR, '.env', 'bin', 'pip'), + path.join(blackInstallDir, '.env', 'bin', 'pip'), ['install', 'black'], - { cwd: Python.BLACK_INSTALL_DIR }, + { cwd: blackInstallDir }, ); } - return path.join(Python.BLACK_INSTALL_DIR, '.env', 'bin', 'black'); + return path.join(blackInstallDir, '.env', 'bin', 'black'); } } diff --git a/packages/jsii-pacmak/test/targets/python.test.ts b/packages/jsii-pacmak/test/targets/python.test.ts index ff700fb1c8..a6f739cb2c 100644 --- a/packages/jsii-pacmak/test/targets/python.test.ts +++ b/packages/jsii-pacmak/test/targets/python.test.ts @@ -9,43 +9,100 @@ import { Rosetta } from 'jsii-rosetta'; describe('python', () => { describe('blackPath', () => { const shellMock = jest.fn(); - const osMock = jest.fn(); - let tmpdir: string; + const homedirMock = jest.fn(); + let homedir: string; + let python: Python; beforeEach(async () => { // eslint-disable-next-line no-import-assign Object.defineProperty(util, 'shell', { value: shellMock }); // eslint-disable-next-line no-import-assign - Object.defineProperty(os, 'homedir', { value: osMock }); - tmpdir = await fs.mkdtemp(path.join(os.tmpdir(), 'jsii-pacmak-black-')); - osMock.mockImplementation(() => tmpdir); + Object.defineProperty(os, 'homedir', { value: homedirMock }); + homedir = await fs.mkdtemp(path.join(os.tmpdir(), 'jsii-pacmak-black-')); + homedirMock.mockImplementation(() => homedir); + python = new Python({ + targetName: 'python', + packageDir: '/dir', + assembly: {} as Assembly, + rosetta: new Rosetta(), + arguments: {}, + }); }); afterEach(async () => { shellMock.mockClear(); - osMock.mockClear(); - await fs.remove(tmpdir); + homedirMock.mockClear(); + await fs.remove(homedir); }); test('black is installed globally', async () => { + let badShellCommand: string | undefined; shellMock.mockImplementation((cmd: string, args: string[], _) => { return new Promise((ok, ko) => { if (cmd === 'which' && args[0] === 'black') { ok('/path/to/black'); + } else { + badShellCommand = `Unexpected call to shell [${cmd} ${args}]`; + ko(badShellCommand); } - ko(`Unexpected call to shell ${cmd} ${args}`); }); }); - const python = new Python({ - targetName: 'python', - packageDir: '/dir', - assembly: {} as Assembly, - rosetta: new Rosetta(), - arguments: {}, - }); - const path = await (python as any).blackPath(); + const path = await (python as any).blackPath(); // call private method blackPath() + expect(badShellCommand).toBeUndefined(); expect(path).toBe('black'); }); + + test('black is installed if not found globally', async () => { + shellMock.mockImplementation((cmd: string, args: string[], _) => { + return new Promise((ok, ko) => { + if (cmd === 'which' && args[0] === 'black') { + ko('black not found'); + } else if ( + /pip.?$/.test(cmd) && + args[0] === 'show' && + args[1] === 'black' + ) { + ko(); + } else { + ok(); + } + }); + }); + + const path = await (python as any).blackPath(); // call private method blackPath() + expect(path).toBe(`${homedir}/.jsii-cache/python-black/.env/bin/black`); + }); + + test('local cache is reused', async () => { + let installCount = 0; + shellMock.mockImplementation((cmd: string, args: string[], _) => { + return new Promise((ok, ko) => { + if (cmd === 'which' && args[0] === 'black') { + ko('black not found'); + } else if ( + /pip.?$/.test(cmd) && + args[0] === 'show' && + args[1] === 'black' + ) { + ko(); + } else if ( + /pip.?$/.test(cmd) && + args[0] === 'install' && + args[1] === 'black' + ) { + installCount++; + ok(); + } else { + ok(); + } + }); + }); + + await (python as any).blackPath(); + await (python as any).blackPath(); + await (python as any).blackPath(); + expect(installCount).toEqual(1); + }); }); });