diff --git a/.github/__tests__/githubUtils.spec.js b/.github/__tests__/githubUtils.spec.js index eb19244bcf3..0a453a8a8eb 100644 --- a/.github/__tests__/githubUtils.spec.js +++ b/.github/__tests__/githubUtils.spec.js @@ -1,24 +1,169 @@ /* eslint-disable import-x/no-commonjs, import-x/no-amd */ 'use strict'; +const { execFileSync } = require('child_process'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); + const { - validateNpmVersionData, - renderNpmVersionMarkdown, + generatePackageVersionData, + validatePackageVersionData, + renderPackageVersionMarkdown, } = require('../githubUtils.js'); -// ---- validateNpmVersionData ---- +// ---- generatePackageVersionData ---- + +function git(cwd, ...args) { + return execFileSync('git', args, { cwd, encoding: 'utf8' }).trim(); +} + +function writeFile(cwd, relPath, contents) { + fs.mkdirSync(path.join(cwd, path.dirname(relPath)), { recursive: true }); + fs.writeFileSync(path.join(cwd, relPath), contents); +} + +function packageJson(name, version, extra = {}) { + return JSON.stringify({ name, version, ...extra }, null, 2); +} + +function pyproject(name, version, classifiers = []) { + return [ + '[build-system]', + 'requires = ["setuptools>=80"]', + '', + '[project]', + `name = "${name}"`, + `version = "${version}"`, + 'authors = [{ name = "Learning Equality", email = "info@learningequality.org" }]', + 'classifiers = [', + ...classifiers.map(c => ` "${c}",`), + ']', + '', + '[project.entry-points."kolibri.plugins"]', + 'version = "not-the-project-version"', + '', + ].join('\n'); +} + +describe('generatePackageVersionData', () => { + let repo; + let originalCwd; + let baseSha; + + beforeEach(() => { + originalCwd = process.cwd(); + repo = fs.mkdtempSync(path.join(os.tmpdir(), 'version-check-')); + git(repo, 'init', '--quiet'); + git(repo, 'config', 'user.email', 'test@example.com'); + git(repo, 'config', 'user.name', 'Test'); + git(repo, 'config', 'commit.gpgsign', 'false'); + + writeFile(repo, 'packages/widget/package.json', packageJson('widget', '1.0.0')); + writeFile(repo, 'packages/widget/index.js', 'module.exports = 1;\n'); + writeFile(repo, 'packages/internal/package.json', packageJson('internal', '1.0.0', { private: true })); + writeFile(repo, 'python_packages/plugin/pyproject.toml', pyproject('plugin', '0.1.0')); + writeFile( + repo, + 'python_packages/workspace-only/pyproject.toml', + pyproject('workspace-only', '0.1.0', ['Framework :: Django', 'Private :: Do Not Upload']) + ); + git(repo, 'add', '.'); + git(repo, 'commit', '--quiet', '--no-verify', '-m', 'fork point'); + + git(repo, 'branch', 'feature'); + + // The base branch moves on after the PR branched off it. + writeFile(repo, 'packages/widget/package.json', packageJson('widget', '2.0.0')); + git(repo, 'commit', '--quiet', '--no-verify', '-a', '-m', 'bump widget on base'); + baseSha = git(repo, 'rev-parse', 'HEAD'); + + git(repo, 'checkout', '--quiet', 'feature'); + process.chdir(repo); + }); + + afterEach(() => { + process.chdir(originalCwd); + fs.rmSync(repo, { recursive: true, force: true }); + }); + + it('returns null when the PR changes no package', () => { + expect(generatePackageVersionData(baseSha)).toBeNull(); + }); + + it('ignores version bumps the base branch made while the PR was behind', () => { + writeFile(repo, 'packages/widget/index.js', 'module.exports = 2;\n'); + git(repo, 'commit', '--quiet', '--no-verify', '-a', '-m', 'edit widget'); + + const data = generatePackageVersionData(baseSha); + + expect(data.packages).toEqual([]); + expect(data.warnings).toEqual([ + { registry: 'npm', name: 'widget', version: '1.0.0', changedFiles: 1 }, + ]); + }); + + it('reports a version bump made by the PR', () => { + writeFile(repo, 'packages/widget/package.json', packageJson('widget', '1.1.0')); + git(repo, 'commit', '--quiet', '--no-verify', '-a', '-m', 'bump widget on feature'); + + const data = generatePackageVersionData(baseSha); + + expect(data.packages).toEqual([ + { registry: 'npm', name: 'widget', from: '1.0.0', to: '1.1.0' }, + ]); + expect(data.warnings).toEqual([]); + }); + + it('reports a bumped Python package against PyPI', () => { + writeFile(repo, 'python_packages/plugin/pyproject.toml', pyproject('plugin', '0.2.0')); + git(repo, 'commit', '--quiet', '--no-verify', '-a', '-m', 'bump plugin'); + + const data = generatePackageVersionData(baseSha); + + expect(data.packages).toEqual([ + { registry: 'PyPI', name: 'plugin', from: '0.1.0', to: '0.2.0' }, + ]); + }); + + it('reports a new package with no previous version', () => { + writeFile(repo, 'python_packages/fresh/pyproject.toml', pyproject('fresh', '0.1.0')); + git(repo, 'add', '.'); + git(repo, 'commit', '--quiet', '--no-verify', '-m', 'add fresh'); -describe('validateNpmVersionData', () => { + const data = generatePackageVersionData(baseSha); + + expect(data.packages).toEqual([ + { registry: 'PyPI', name: 'fresh', from: null, to: '0.1.0' }, + ]); + }); + + it('ignores packages marked unpublishable', () => { + writeFile(repo, 'packages/internal/package.json', packageJson('internal', '1.1.0', { private: true })); + writeFile( + repo, + 'python_packages/workspace-only/pyproject.toml', + pyproject('workspace-only', '0.2.0', ['Framework :: Django', 'Private :: Do Not Upload']) + ); + git(repo, 'commit', '--quiet', '--no-verify', '-a', '-m', 'bump unpublishable packages'); + + expect(generatePackageVersionData(baseSha)).toBeNull(); + }); +}); + +// ---- validatePackageVersionData ---- + +describe('validatePackageVersionData', () => { it('returns null for JSON null (no packages changed)', () => { - expect(validateNpmVersionData('null')).toBeNull(); + expect(validatePackageVersionData('null')).toBeNull(); }); it('accepts valid data with packages and warnings', () => { const raw = JSON.stringify({ - packages: [{ name: 'my-lib', from: '1.0.0', to: '1.1.0' }], - warnings: [{ name: 'other-pkg', version: '2.0.0', changedFiles: 3 }], + packages: [{ registry: 'npm', name: 'my-lib', from: '1.0.0', to: '1.1.0' }], + warnings: [{ registry: 'PyPI', name: 'other-pkg', version: '2.0.0', changedFiles: 3 }], }); - const data = validateNpmVersionData(raw); + const data = validatePackageVersionData(raw); expect(data.packages).toHaveLength(1); expect(data.packages[0].name).toBe('my-lib'); expect(data.warnings).toHaveLength(1); @@ -26,92 +171,109 @@ describe('validateNpmVersionData', () => { it('accepts package with from: null (new package)', () => { const raw = JSON.stringify({ - packages: [{ name: '@foo/new', from: null, to: '1.0.0' }], + packages: [{ registry: 'npm', name: '@foo/new', from: null, to: '1.0.0' }], warnings: [], }); - expect(validateNpmVersionData(raw).packages[0].from).toBeNull(); + expect(validatePackageVersionData(raw).packages[0].from).toBeNull(); }); it('throws on invalid JSON', () => { - expect(() => validateNpmVersionData('not-json')).toThrow(); + expect(() => validatePackageVersionData('not-json')).toThrow(); }); it('throws when packages is not an array', () => { const raw = JSON.stringify({ packages: 'oops', warnings: [] }); - expect(() => validateNpmVersionData(raw)).toThrow(); + expect(() => validatePackageVersionData(raw)).toThrow(); }); it('throws when warnings is not an array', () => { const raw = JSON.stringify({ packages: [], warnings: 'oops' }); - expect(() => validateNpmVersionData(raw)).toThrow(); + expect(() => validatePackageVersionData(raw)).toThrow(); + }); + + it('throws when package.registry is not a string', () => { + const raw = JSON.stringify({ + packages: [{ registry: 42, name: 'x', from: '1.0', to: '1.1' }], + warnings: [], + }); + expect(() => validatePackageVersionData(raw)).toThrow(); }); it('throws when package.name is not a string', () => { - const raw = JSON.stringify({ packages: [{ name: 42, from: '1.0', to: '1.1' }], warnings: [] }); - expect(() => validateNpmVersionData(raw)).toThrow(); + const raw = JSON.stringify({ + packages: [{ registry: 'npm', name: 42, from: '1.0', to: '1.1' }], + warnings: [], + }); + expect(() => validatePackageVersionData(raw)).toThrow(); }); it('throws when package.from is not string or null', () => { - const raw = JSON.stringify({ packages: [{ name: 'x', from: 42, to: '1.1' }], warnings: [] }); - expect(() => validateNpmVersionData(raw)).toThrow(); + const raw = JSON.stringify({ + packages: [{ registry: 'npm', name: 'x', from: 42, to: '1.1' }], + warnings: [], + }); + expect(() => validatePackageVersionData(raw)).toThrow(); }); it('throws when package.to is not a string', () => { - const raw = JSON.stringify({ packages: [{ name: 'x', from: null, to: 42 }], warnings: [] }); - expect(() => validateNpmVersionData(raw)).toThrow(); + const raw = JSON.stringify({ + packages: [{ registry: 'npm', name: 'x', from: null, to: 42 }], + warnings: [], + }); + expect(() => validatePackageVersionData(raw)).toThrow(); }); it('throws when warning.changedFiles is not a number', () => { const raw = JSON.stringify({ packages: [], - warnings: [{ name: 'x', version: '1.0', changedFiles: 'three' }], + warnings: [{ registry: 'npm', name: 'x', version: '1.0', changedFiles: 'three' }], }); - expect(() => validateNpmVersionData(raw)).toThrow(); + expect(() => validatePackageVersionData(raw)).toThrow(); }); }); -// ---- renderNpmVersionMarkdown ---- +// ---- renderPackageVersionMarkdown ---- -describe('renderNpmVersionMarkdown', () => { +describe('renderPackageVersionMarkdown', () => { it('returns null for empty data', () => { - expect(renderNpmVersionMarkdown({ packages: [], warnings: [] })).toBeNull(); + expect(renderPackageVersionMarkdown({ packages: [], warnings: [] })).toBeNull(); }); it('renders publish table for bumped packages', () => { - const data = { packages: [{ name: '@foo/bar', from: '1.0.0', to: '1.1.0' }], warnings: [] }; - const result = renderNpmVersionMarkdown(data); - expect(result).toContain('npm Package Versions'); - expect(result).toContain('@foo/bar'); - expect(result).toContain('1.0.0'); - expect(result).toContain('1.1.0'); + const data = { + packages: [{ registry: 'npm', name: '@foo/bar', from: '1.0.0', to: '1.1.0' }], + warnings: [], + }; + const result = renderPackageVersionMarkdown(data); + expect(result).toContain('Package Versions'); + expect(result).toContain('| @foo/bar | npm | 1.0.0 | 1.1.0 |'); }); it('renders _new_ for new packages (from: null)', () => { - const data = { packages: [{ name: '@foo/new', from: null, to: '1.0.0' }], warnings: [] }; - const result = renderNpmVersionMarkdown(data); - expect(result).toContain('_new_'); - expect(result).toContain('@foo/new'); - expect(result).toContain('1.0.0'); + const data = { + packages: [{ registry: 'PyPI', name: 'kolibri-new-plugin', from: null, to: '1.0.0' }], + warnings: [], + }; + const result = renderPackageVersionMarkdown(data); + expect(result).toContain('| kolibri-new-plugin | PyPI | _new_ | 1.0.0 |'); }); it('renders warning section with changed file count', () => { const data = { packages: [], - warnings: [{ name: '@foo/baz', version: '2.0.0', changedFiles: 5 }], + warnings: [{ registry: 'npm', name: '@foo/baz', version: '2.0.0', changedFiles: 5 }], }; - const result = renderNpmVersionMarkdown(data); + const result = renderPackageVersionMarkdown(data); expect(result).toContain('WARNING'); - expect(result).toContain('@foo/baz'); - expect(result).toContain('2.0.0'); - expect(result).toContain('5'); + expect(result).toContain('| @foo/baz | npm | 2.0.0 | 5 |'); }); it('renders both sections when there are packages and warnings', () => { const data = { - packages: [{ name: 'my-lib', from: '1.0.0', to: '1.1.0' }], - warnings: [{ name: 'other-pkg', version: '2.0.0', changedFiles: 2 }], + packages: [{ registry: 'npm', name: 'my-lib', from: '1.0.0', to: '1.1.0' }], + warnings: [{ registry: 'PyPI', name: 'other-pkg', version: '2.0.0', changedFiles: 2 }], }; - const result = renderNpmVersionMarkdown(data); + const result = renderPackageVersionMarkdown(data); expect(result).toContain('my-lib'); expect(result).toContain('WARNING'); expect(result).toContain('other-pkg'); diff --git a/.github/githubUtils.js b/.github/githubUtils.js index 425114c3823..a40e3b64ab9 100644 --- a/.github/githubUtils.js +++ b/.github/githubUtils.js @@ -167,49 +167,107 @@ async function uploadReleaseAsset(github, context, filePath, release_id) { }); } -const npmVersionsHeader = '**npm Package Versions**'; +const packageVersionsHeader = '**Package Versions**'; /** - * Generate structured version diff data for packages changed relative to baseSha. - * Returns { packages: [{name, from, to}], warnings: [{name, version, changedFiles}] }, + * Read name and version from an npm package.json. + * Returns null when the package is not publishable. + */ +function readPackageJson(contents) { + const pkg = JSON.parse(contents); + if (pkg.private === true || pkg.private === 'true') return null; + if (!pkg.name || !pkg.version) return null; + return { name: pkg.name, version: pkg.version }; +} + +/** + * Read name and version from a pyproject.toml's [project] table. + * Returns null when the package is not publishable — PyPI rejects any package + * carrying a `Private ::` classifier, which is how a workspace member opts out. + */ +function readPyproject(contents) { + const projectLines = []; + let inProject = false; + for (const line of contents.split('\n')) { + const header = line.match(/^\s*\[\[?([^[\]]+)\]\]?\s*$/); + if (header) { + inProject = header[1] === 'project'; + continue; + } + if (inProject) projectLines.push(line); + } + const project = projectLines.join('\n'); + + const classifiers = project.match(/^\s*classifiers\s*=\s*\[([^\]]*)\]/m); + if (classifiers && /["']\s*Private\s*::/.test(classifiers[1])) return null; + + const name = project.match(/^\s*name\s*=\s*["']([^"']+)["']/m); + const version = project.match(/^\s*version\s*=\s*["']([^"']+)["']/m); + if (!name || !version) return null; + return { name: name[1], version: version[1] }; +} + +const PACKAGE_ECOSYSTEMS = [ + { registry: 'npm', root: 'packages', manifest: 'package.json', read: readPackageJson }, + { registry: 'PyPI', root: 'python_packages', manifest: 'pyproject.toml', read: readPyproject }, +]; + +/** + * Generate structured version diff data for packages the PR itself changed. + * Returns { packages: [{registry, name, from, to}], + * warnings: [{registry, name, version, changedFiles}] }, * or null if no publishable packages were affected. - * Runs in pull_request context (no write permissions needed). + * Runs in pull_request context (no write permissions needed), against a + * checkout of the PR head. * `from` is null for new packages. */ -function generateNpmVersionData(baseSha) { - const allChanged = execSync(`git diff --name-only ${baseSha} -- packages/`) - .toString().trim().split('\n').filter(Boolean); - - const changedByPkg = {}; - for (const file of allChanged) { - const parts = file.split('/'); - if (parts.length < 2) continue; - const pkgDir = parts.slice(0, 2).join('/'); - changedByPkg[pkgDir] = (changedByPkg[pkgDir] || 0) + 1; - } +function generatePackageVersionData(baseSha) { + // A pull_request payload's base.sha is the base branch tip, not the fork + // point, so diffing straight from it reports the base's own changes back at + // a PR that is behind. + const mergeBase = execSync(`git merge-base ${baseSha} HEAD`).toString().trim(); const packages = []; const warnings = []; - for (const [pkgDir, fileCount] of Object.entries(changedByPkg)) { - const pkgJsonPath = path.join(pkgDir, 'package.json'); - if (!fs.existsSync(pkgJsonPath)) continue; + for (const { registry, root, manifest, read } of PACKAGE_ECOSYSTEMS) { + const allChanged = execSync(`git diff --name-only ${mergeBase} -- ${root}/`) + .toString().trim().split('\n').filter(Boolean); - const newPkg = JSON.parse(fs.readFileSync(pkgJsonPath, 'utf8')); - if (newPkg.private === true || newPkg.private === 'true') continue; - - let oldPkg; - try { - oldPkg = JSON.parse(execSync(`git show ${baseSha}:${pkgJsonPath}`, { encoding: 'utf8' })); - } catch { - packages.push({ name: newPkg.name, from: null, to: newPkg.version }); - continue; + const changedByPkg = {}; + for (const file of allChanged) { + const parts = file.split('/'); + if (parts.length < 2) continue; + const pkgDir = parts.slice(0, 2).join('/'); + changedByPkg[pkgDir] = (changedByPkg[pkgDir] || 0) + 1; } - if (oldPkg.version !== newPkg.version) { - packages.push({ name: newPkg.name, from: oldPkg.version, to: newPkg.version }); - } else { - warnings.push({ name: newPkg.name, version: newPkg.version, changedFiles: fileCount }); + for (const [pkgDir, changedFiles] of Object.entries(changedByPkg)) { + const manifestPath = path.join(pkgDir, manifest); + if (!fs.existsSync(manifestPath)) continue; + + const current = read(fs.readFileSync(manifestPath, 'utf8')); + if (!current) continue; + + let previous = null; + try { + previous = read( + execSync(`git show ${mergeBase}:${manifestPath}`, { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'ignore'], + }) + ); + } catch { + previous = null; + } + + if (!previous) { + packages.push({ registry, name: current.name, from: null, to: current.version }); + } else if (previous.version !== current.version) { + packages.push({ registry, name: current.name, from: previous.version, to: current.version }); + } else { + warnings.push({ registry, name: current.name, version: current.version, changedFiles }); + } } } @@ -224,7 +282,7 @@ function generateNpmVersionData(baseSha) { * Returns the parsed data object, or null if the JSON value is null (no packages changed). * Throws an Error with a descriptive message on any malformed input. */ -function validateNpmVersionData(raw) { +function validatePackageVersionData(raw) { const data = JSON.parse(raw); if (data === null) return null; if (typeof data !== 'object' || Array.isArray(data)) { @@ -233,6 +291,7 @@ function validateNpmVersionData(raw) { if (!Array.isArray(data.packages)) throw new Error('packages must be an array'); if (!Array.isArray(data.warnings)) throw new Error('warnings must be an array'); for (const pkg of data.packages) { + if (typeof pkg.registry !== 'string') throw new Error('package.registry must be a string'); if (typeof pkg.name !== 'string') throw new Error('package.name must be a string'); if (pkg.from !== null && typeof pkg.from !== 'string') { throw new Error('package.from must be a string or null'); @@ -240,6 +299,7 @@ function validateNpmVersionData(raw) { if (typeof pkg.to !== 'string') throw new Error('package.to must be a string'); } for (const w of data.warnings) { + if (typeof w.registry !== 'string') throw new Error('warning.registry must be a string'); if (typeof w.name !== 'string') throw new Error('warning.name must be a string'); if (typeof w.version !== 'string') throw new Error('warning.version must be a string'); if (typeof w.changedFiles !== 'number') throw new Error('warning.changedFiles must be a number'); @@ -251,46 +311,46 @@ function validateNpmVersionData(raw) { * Render the markdown comment body from validated version diff data. * Returns the markdown string, or null if there is nothing to report. */ -function renderNpmVersionMarkdown(data) { +function renderPackageVersionMarkdown(data) { const publishRows = data.packages.map( - pkg => `| ${pkg.name} | ${pkg.from === null ? '_new_' : pkg.from} | ${pkg.to} |` + pkg => `| ${pkg.name} | ${pkg.registry} | ${pkg.from === null ? '_new_' : pkg.from} | ${pkg.to} |` ); const warningRows = data.warnings.map( - w => `| ${w.name} | ${w.version} | ${w.changedFiles} |` + w => `| ${w.name} | ${w.registry} | ${w.version} | ${w.changedFiles} |` ); const sections = []; if (publishRows.length) { sections.push( - `Merging this PR will publish the following packages to npm:\n\n` + - `| Package | Current | New |\n|-|-|-|\n${publishRows.join('\n')}` + `Merging this PR will publish the following packages:\n\n` + + `| Package | Registry | Current | New |\n|-|-|-|-|\n${publishRows.join('\n')}` ); } if (warningRows.length) { sections.push( `> [!WARNING]\n` + `> The following packages have changed files but no version bump:\n\n` + - `| Package | Version | Changed files |\n|-|-|-|\n${warningRows.join('\n')}\n\n` + + `| Package | Registry | Version | Changed files |\n|-|-|-|-|\n${warningRows.join('\n')}\n\n` + `If these changes affect published code, consider bumping the version.` ); } if (sections.length) { - return `### ${npmVersionsHeader}\n\n${sections.join('\n\n')}`; + return `### ${packageVersionsHeader}\n\n${sections.join('\n\n')}`; } return null; } /** - * Post or update the npm version check comment on a PR. Runs in + * Post or update the package version check comment on a PR. Runs in * workflow_run context (with write permissions). Pass body from - * renderNpmVersionMarkdown, or null to delete any existing comment. + * renderPackageVersionMarkdown, or null to delete any existing comment. */ -async function postNpmVersionComment(github, context, prNumber, body) { +async function postPackageVersionComment(github, context, prNumber, body) { if (body) { - await upsertComment(github, context, prNumber, npmVersionsHeader, body); + await upsertComment(github, context, prNumber, packageVersionsHeader, body); } else { - const commentId = await findComment(github, context, prNumber, npmVersionsHeader); + const commentId = await findComment(github, context, prNumber, packageVersionsHeader); if (commentId) { await github.rest.issues.deleteComment({ owner: context.repo.owner, @@ -345,10 +405,10 @@ module.exports = { findComment, findPrByHeadSha, generateAssetComment, - generateNpmVersionData, - validateNpmVersionData, - renderNpmVersionMarkdown, - postNpmVersionComment, + generatePackageVersionData, + validatePackageVersionData, + renderPackageVersionMarkdown, + postPackageVersionComment, uploadReleaseAsset, upsertComment, } diff --git a/.github/workflows/npm_version_check.yml b/.github/workflows/package_version_check.yml similarity index 58% rename from .github/workflows/npm_version_check.yml rename to .github/workflows/package_version_check.yml index 52762840f3c..82839657f5a 100644 --- a/.github/workflows/npm_version_check.yml +++ b/.github/workflows/package_version_check.yml @@ -1,8 +1,9 @@ -name: npm Package Version Check +name: Package Version Check on: pull_request: paths: - 'packages/**' + - 'python_packages/**' concurrency: group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} cancel-in-progress: true @@ -15,6 +16,9 @@ jobs: steps: - uses: actions/checkout@v7.0.1 with: + # The PR head rather than the default merge commit, so the report + # covers only what the PR itself changed. + ref: ${{ github.event.pull_request.head.sha }} fetch-depth: 0 - uses: actions/setup-node@v7 with: @@ -26,10 +30,10 @@ jobs: const fs = require('fs'); const utils = require('./.github/githubUtils.js'); const base = context.payload.pull_request.base.sha; - const data = utils.generateNpmVersionData(base); - fs.mkdirSync('npm_version_report', { recursive: true }); - fs.writeFileSync('npm_version_report/data.json', JSON.stringify(data)); + const data = utils.generatePackageVersionData(base); + fs.mkdirSync('package_version_report', { recursive: true }); + fs.writeFileSync('package_version_report/data.json', JSON.stringify(data)); - uses: actions/upload-artifact@v7 with: - name: npm_version_report - path: npm_version_report/ + name: package_version_report + path: package_version_report/ diff --git a/.github/workflows/npm_version_comment.yml b/.github/workflows/package_version_comment.yml similarity index 79% rename from .github/workflows/npm_version_comment.yml rename to .github/workflows/package_version_comment.yml index b30a0cabe92..ef8f4b6eac0 100644 --- a/.github/workflows/npm_version_comment.yml +++ b/.github/workflows/package_version_comment.yml @@ -1,7 +1,7 @@ -name: npm Package Version Comment +name: Package Version Comment on: workflow_run: - workflows: [npm Package Version Check] + workflows: [Package Version Check] types: - completed permissions: @@ -32,8 +32,8 @@ jobs: if: steps.find-pr.outputs.found == 'true' uses: actions/download-artifact@v8 with: - name: npm_version_report - path: ${{ runner.temp }}/npm_version_report + name: package_version_report + path: ${{ runner.temp }}/package_version_report github-token: ${{ github.token }} repository: ${{ github.repository }} run-id: ${{ github.event.workflow_run.id }} @@ -42,7 +42,7 @@ jobs: uses: actions/github-script@v9 env: PR_NUMBER: ${{ steps.find-pr.outputs.result }} - REPORT_DIR: ${{ runner.temp }}/npm_version_report + REPORT_DIR: ${{ runner.temp }}/package_version_report with: script: | const fs = require('fs'); @@ -51,11 +51,11 @@ jobs: const raw = fs.readFileSync(path.join(process.env.REPORT_DIR, 'data.json'), 'utf8'); let data; try { - data = utils.validateNpmVersionData(raw); + data = utils.validatePackageVersionData(raw); } catch (e) { core.setFailed(`Invalid version data JSON: ${e.message}`); return; } - const body = data ? utils.renderNpmVersionMarkdown(data) : null; + const body = data ? utils.renderPackageVersionMarkdown(data) : null; const prNumber = Number(process.env.PR_NUMBER); - await utils.postNpmVersionComment(github, context, prNumber, body); + await utils.postPackageVersionComment(github, context, prNumber, body); diff --git a/.github/workflows/pypi_packages_publish.yml b/.github/workflows/pypi_packages_publish.yml index bdda1550b24..358de933d0a 100644 --- a/.github/workflows/pypi_packages_publish.yml +++ b/.github/workflows/pypi_packages_publish.yml @@ -4,11 +4,7 @@ on: branches: - develop paths: - - 'python_packages/kolibri-sync-extras-plugin/pyproject.toml' - - 'python_packages/kolibri-sentry-plugin/pyproject.toml' - - 'python_packages/kolibri-oidc-client-plugin/pyproject.toml' - - 'python_packages/kolibri-oidc-provider-plugin/pyproject.toml' - - 'python_packages/kolibri-opensearch-plugin/pyproject.toml' + - 'python_packages/*/pyproject.toml' workflow_dispatch: inputs: pypi_package: @@ -18,6 +14,8 @@ on: default: 'all' options: - 'all' + - 'kolibri-context-translation-plugin' + - 'kolibri-demo-server-plugin' - 'kolibri-sync-extras-plugin' - 'kolibri-sentry-plugin' - 'kolibri-oidc-client-plugin' diff --git a/docs/howtos/python_monorepo.md b/docs/howtos/python_monorepo.md index ae56ed05e94..811b7b8925a 100644 --- a/docs/howtos/python_monorepo.md +++ b/docs/howtos/python_monorepo.md @@ -18,14 +18,26 @@ Kolibri's Python code is organized as a [uv workspace](https://docs.astral.sh/uv Member package versions are independent of each other and of the main `kolibri` package — there's no enforcement linking them. Use a static `version = "x.y.z"` field, not `setuptools-scm`-derived dynamic versioning: this repo's git tags are Kolibri's own release tags, so dynamic versioning inside the workspace would report Kolibri's version instead of the package's own. -## Marking a package as publishable +## Publishing a package -By default, a package under `python_packages/` is workspace-only — nothing publishes it. To publish it to PyPI: +Packages under `python_packages/` are published to PyPI by default. To publish a new one: 1. Add a `Makefile` with a `dist` target that builds the wheel: `uv build -o dist` for a backend-only package, or (for a package with a frontend bundle) `pnpm run build && pnpm run compress && uv build -o dist` — see `python_packages/kolibri-sentry-plugin/Makefile` for the frontend case. `scripts/pypi_publish.sh` calls `make -C python_packages/ dist` and publishes whatever lands in that member's own `dist/`. -2. Add its `pyproject.toml` path to the `paths:` filter in `.github/workflows/pypi_packages_publish.yml`'s `push` trigger. -3. Add its name to the `workflow_dispatch.inputs.pypi_package.options` list in the same file. -4. Register a pending trusted publisher on PyPI and TestPyPI (see the "Python packages" section of [the release process docs](../release_process.rst)) before merging. +2. Add its name to the `workflow_dispatch.inputs.pypi_package.options` list in `.github/workflows/pypi_packages_publish.yml`, so it can be published manually as well as on merge. +3. Register a pending trusted publisher on PyPI and TestPyPI (see the "Python packages" section of [the release process docs](../release_process.rst)) before merging. + +## Keeping a package off PyPI + +Declare the [`Private :: Do Not Upload`](https://packaging.python.org/en/latest/guides/writing-pyproject-toml/) classifier — PyPI rejects any distribution carrying a `Private ::` classifier, and both `scripts/pypi_publish.sh` and the PR version check skip such packages: + +```toml +[project] +classifiers = [ + "Private :: Do Not Upload", +] +``` + +This is the Python equivalent of `"private": true` in an npm `package.json`. ## CI cascade diff --git a/docs/release_process.rst b/docs/release_process.rst index 3f0ec2742c5..26234ceb444 100644 --- a/docs/release_process.rst +++ b/docs/release_process.rst @@ -62,12 +62,12 @@ Every publish includes an SLSA provenance attestation linking the npm version to Python packages =============== -Packages in ``python_packages/`` are published to PyPI independently of Kolibri releases and independently of each other. Only publishable packages are published; everything else is workspace-only. A package is publishable if it's listed in ``pypi_packages_publish.yml``'s ``paths:`` filter and ``workflow_dispatch`` options. +Packages in ``python_packages/`` are published to PyPI independently of Kolibri releases and independently of each other. A package can opt out by declaring the ``Private :: Do Not Upload`` classifier in its ``pyproject.toml`` — none currently do. PyPI rejects any distribution carrying a ``Private ::`` classifier, so the marker is enforced server-side as well. Automatic publishing -------------------- -When code merging to ``develop`` changes a listed package's ``pyproject.toml``, the ``pypi_packages_publish.yml`` workflow compares that package's version against PyPI and publishes it if newer. +When code merging to ``develop`` changes a package's ``pyproject.toml``, the ``pypi_packages_publish.yml`` workflow compares that package's version against PyPI and publishes it if newer. Authentication uses PyPI OIDC trusted publishing (no API tokens). diff --git a/scripts/pypi_publish.sh b/scripts/pypi_publish.sh index 12603ba01cc..84b887fdcaf 100755 --- a/scripts/pypi_publish.sh +++ b/scripts/pypi_publish.sh @@ -21,13 +21,29 @@ fi # need to re-run `uv version` for packages already resolved during detection. TO_PUBLISH=() +# PyPI rejects any distribution carrying a "Private ::" classifier, so a +# workspace member opts out of publishing by declaring one. +is_private() { + grep -q '"Private *::' "$1/pyproject.toml" +} + if [ $# -ge 1 ]; then + if is_private "python_packages/$1"; then + echo "Refusing to publish $1: marked Private :: Do Not Upload" + exit 1 + fi TO_PUBLISH=("$1:$(uv version --package "$1" --short)") echo "Publishing $1" else for pkg_dir in python_packages/*/; do [ -f "$pkg_dir/pyproject.toml" ] || continue name=$(basename "$pkg_dir") + + if is_private "$pkg_dir"; then + echo "Skipping $name (Private :: Do Not Upload)" + continue + fi + repo_version=$(uv version --package "$name" --short) # Unlike scripts/npm_publish.sh, a 404 here is never skipped in CI: npm