Add License In build#5
Conversation
Summary of ChangesHello @0suu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust system for managing and distributing third-party software licenses. By automating the generation of a comprehensive license file and integrating it directly into the build and packaging process, the project ensures legal compliance and transparency regarding its dependencies. This change streamlines the maintenance of license information and makes it readily available within the application's installer. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "from": "build/licenses", | ||
| "to": "licenses", | ||
| "filter": [ | ||
| "THIRD_PARTY_LICENSES.txt" | ||
| ] |
There was a problem hiding this comment.
The new extraResources entry instructs electron-builder to copy build/licenses/THIRD_PARTY_LICENSES.txt, but the existing npm scripts still only call build:vite and build:electron—there is no build:licenses task or other hook that runs scripts/generate-licenses.js, and build/licenses is not present in the repo. When npm run package triggers electron-builder, it will fail with an ENOENT because the from path in this extra resource does not exist. Please wire license generation into the build/package pipeline (or commit the file) so installers can be produced.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds functionality to generate and include third-party licenses in the application build. It introduces a new script for license generation, adds the necessary dependency, and configures electron-builder to bundle the licenses. While the implementation is mostly correct, there is a critical issue: the main build script has not been updated to run the new license generation script, which contradicts the updated documentation and will cause the packaging step to fail. I've provided comments on the README files with details on how to resolve this.
|
|
||
| Electron Builder の設定 (`package.json` の `build` フィールド) により、アプリ ID や配布ターゲット、アイコンなどが制御されます。 | ||
|
|
||
| 依存ライブラリのライセンス一覧は `npm run build:licenses`（`npm run build` 内で自動実行）で `build/licenses/THIRD_PARTY_LICENSES.txt` に生成され、ルートの `LICENSE` と共にインストーラへ同梱されます。依存パッケージを更新した際はこのスクリプトを再実行してください。 |
There was a problem hiding this comment.
This documentation states that npm run build:licenses is automatically executed within npm run build, but the scripts in package.json have not been updated to reflect this.
The build:licenses script needs to be defined and called from the build script. Without this change, THIRD_PARTY_LICENSES.txt will not be generated, causing electron-builder to fail during the npm run package command.
Please update package.json to integrate the license generation. For example:
"scripts": {
...
"build": "npm run build:licenses && npm run build:vite && npm run build:electron",
"build:licenses": "node scripts/generate-licenses.js",
...
}|
|
||
| Electron Builder uses the configuration defined in `package.json` (`build` field) for app ID, product name, and targets. | ||
|
|
||
| Dependency licenses are generated via `npm run build:licenses` (automatically executed inside `npm run build`) into `build/licenses/THIRD_PARTY_LICENSES.txt`, and Electron Builder bundles that file together with the root `LICENSE` in the installers. Re-run the script after dependency changes to refresh the list. |
There was a problem hiding this comment.
This documentation states that npm run build:licenses is "automatically executed inside npm run build". However, the scripts in package.json have not been updated to implement this.
The build:licenses script is missing, and the main build script does not call it. This will cause the npm run package command to fail because electron-builder will not be able to find the THIRD_PARTY_LICENSES.txt file.
Please update package.json to add the build:licenses script and integrate it into the build script. For example:
"scripts": {
...
"build": "npm run build:licenses && npm run build:vite && npm run build:electron",
"build:licenses": "node scripts/generate-licenses.js",
...
}
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request adds a mechanism to generate a THIRD_PARTY_LICENSES.txt file and bundle it into the application build, which is great for compliance. The implementation is solid, using license-checker and integrating it into the build process via an npm script. My review includes two suggestions for improvement: one regarding the long-term maintainability of the chosen dependency, and another to refactor the new script to use asynchronous file system operations, which is a best practice in Node.js.
| "identity-obj-proxy": "^3.0.0", | ||
| "jest": "^29.7.0", | ||
| "jest-environment-jsdom": "^29.7.0", | ||
| "license-checker": "^25.0.1", |
There was a problem hiding this comment.
The license-checker package is a good choice for this task, but this version (25.0.1) was published several years ago and pulls in multiple deprecated dependencies (e.g., read-installed, osenv, debuglog). While it works for now, this could lead to maintenance or security issues in the future. For better long-term stability, you might consider evaluating a more modern and actively maintained alternative, such as license-checker-rseidelsohn or npm-license-crawler.
| const fs = require('fs'); | ||
| const path = require('path'); | ||
| const util = require('util'); | ||
| const checker = require('license-checker'); | ||
|
|
||
| const init = util.promisify(checker.init); | ||
| const projectRoot = path.resolve(__dirname, '..'); | ||
| const outputDir = path.join(projectRoot, 'build', 'licenses'); | ||
| const outputFile = path.join(outputDir, 'THIRD_PARTY_LICENSES.txt'); | ||
| const packageJson = require(path.join(projectRoot, 'package.json')); | ||
|
|
||
| function readLicenseFile(filePath) { | ||
| if (!filePath) { | ||
| return ''; | ||
| } | ||
|
|
||
| try { | ||
| return fs.readFileSync(filePath, 'utf8'); | ||
| } catch (error) { | ||
| return ''; | ||
| } | ||
| } | ||
|
|
||
| function formatPackage(pkgName, info) { | ||
| const license = Array.isArray(info.licenses) | ||
| ? info.licenses.join(', ') | ||
| : info.licenses || 'UNKNOWN'; | ||
| const publisher = info.publisher || 'N/A'; | ||
| const repository = info.repository || info.url || 'N/A'; | ||
| const licenseFile = info.licenseFile ? path.relative(projectRoot, info.licenseFile) : 'N/A'; | ||
| const licenseText = (info.licenseText || readLicenseFile(info.licenseFile) || 'License text unavailable.').trim(); | ||
|
|
||
| return [ | ||
| pkgName, | ||
| `License: ${license}`, | ||
| `Publisher: ${publisher}`, | ||
| `Repository: ${repository}`, | ||
| `License file: ${licenseFile}`, | ||
| '', | ||
| licenseText, | ||
| ].join('\n'); | ||
| } | ||
|
|
||
| async function main() { | ||
| const skipSelf = packageJson.name && packageJson.version ? `${packageJson.name}@${packageJson.version}` : null; | ||
| const packages = await init({ | ||
| start: projectRoot, | ||
| production: true, | ||
| licenseText: true, | ||
| }); | ||
|
|
||
| const entries = Object.keys(packages) | ||
| .filter((pkgName) => pkgName !== skipSelf) | ||
| .sort((a, b) => a.localeCompare(b)) | ||
| .map((pkgName) => formatPackage(pkgName, packages[pkgName])); | ||
|
|
||
| const header = [ | ||
| `Third-party licenses for ${packageJson.name || 'application'} v${packageJson.version || ''}`.trim(), | ||
| `Generated at ${new Date().toISOString()}`, | ||
| '', | ||
| ].join('\n'); | ||
|
|
||
| fs.mkdirSync(outputDir, { recursive: true }); | ||
| fs.writeFileSync(outputFile, `${header}${entries.join('\n\n-----\n\n')}\n`, 'utf8'); | ||
| console.log(`Saved license report for ${entries.length} dependencies to ${path.relative(projectRoot, outputFile)}`); | ||
| } | ||
|
|
||
| main().catch((error) => { | ||
| console.error('[license-generation] Failed to generate license report.'); | ||
| console.error(error); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
While using synchronous file system methods (readFileSync, mkdirSync, writeFileSync) is acceptable in a simple build script, it's a good practice in Node.js to use their asynchronous counterparts to avoid blocking the event loop. Since this script is already using async/await, you can refactor it to use fs.promises for better consistency and performance, especially when reading multiple license files.
const fs = require('fs').promises;
const path = require('path');
const util = require('util');
const checker = require('license-checker');
const init = util.promisify(checker.init);
const projectRoot = path.resolve(__dirname, '..');
const outputDir = path.join(projectRoot, 'build', 'licenses');
const outputFile = path.join(outputDir, 'THIRD_PARTY_LICENSES.txt');
const packageJson = require(path.join(projectRoot, 'package.json'));
async function readLicenseFile(filePath) {
if (!filePath) {
return '';
}
try {
return await fs.readFile(filePath, 'utf8');
} catch (error) {
// It's okay if a license file doesn't exist, so we don't need to log this error.
return '';
}
}
async function formatPackage(pkgName, info) {
const license = Array.isArray(info.licenses)
? info.licenses.join(', ')
: info.licenses || 'UNKNOWN';
const publisher = info.publisher || 'N/A';
const repository = info.repository || info.url || 'N/A';
const licenseFile = info.licenseFile ? path.relative(projectRoot, info.licenseFile) : 'N/A';
const licenseTextFromFile = await readLicenseFile(info.licenseFile);
const licenseText = (info.licenseText || licenseTextFromFile || 'License text unavailable.').trim();
return [
pkgName,
`License: ${license}`,
`Publisher: ${publisher}`,
`Repository: ${repository}`,
`License file: ${licenseFile}`,
'',
licenseText,
].join('\n');
}
async function main() {
const skipSelf = packageJson.name && packageJson.version ? `${packageJson.name}@${packageJson.version}` : null;
const packages = await init({
start: projectRoot,
production: true,
licenseText: true,
});
const packageNames = Object.keys(packages)
.filter((pkgName) => pkgName !== skipSelf)
.sort((a, b) => a.localeCompare(b));
const entries = await Promise.all(
packageNames.map((pkgName) => formatPackage(pkgName, packages[pkgName]))
);
const header = [
`Third-party licenses for ${packageJson.name || 'application'} v${packageJson.version || ''}`.trim(),
`Generated at ${new Date().toISOString()}`,
'',
].join('\n');
await fs.mkdir(outputDir, { recursive: true });
await fs.writeFile(outputFile, `${header}${entries.join('\n\n-----\n\n')}\n`, 'utf8');
console.log(`Saved license report for ${entries.length} dependencies to ${path.relative(projectRoot, outputFile)}`);
}
main().catch((error) => {
console.error('[license-generation] Failed to generate license report.');
console.error(error);
process.exit(1);
});
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const packages = await init({ | ||
| start: projectRoot, | ||
| production: true, | ||
| licenseText: true, |
There was a problem hiding this comment.
generate-licenses.js runs license-checker with production: true, which omits devDependencies. In this repo React/ReactDOM live in devDependencies (package.json) but their code is bundled into the renderer output. Running npm run build now produces build/licenses/THIRD_PARTY_LICENSES.txt without those licenses, so installers miss notices for packages actually shipped with the app.
Useful? React with 👍 / 👎.
|
/gemini review |
|
/codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to automatically generate and bundle third-party dependency licenses into the application build. This is achieved by adding the license-checker-rseidelsohn package and a new Node.js script (scripts/generate-licenses.js) that runs as part of the npm run build command. The changes also include updating the README files to document this new process and correctly categorizing react and react-dom as runtime dependencies.
The implementation is well-done and robust. I have one suggestion to improve the error logging in the new license generation script to make it easier to debug potential issues with missing license files in the future. Overall, this is a great addition for license compliance.
| try { | ||
| return fs.readFileSync(filePath, 'utf8'); | ||
| } catch (error) { | ||
| return ''; |
There was a problem hiding this comment.
The current implementation silently ignores errors when reading a license file. While this prevents the build from failing, it hides potentially important issues, such as a license file being referenced but missing or unreadable. It would be more robust to log a warning to the console when an error occurs. This will improve the debuggability of the license generation process without breaking the build.
console.warn(`[license-generation] Could not read license file at ${filePath}: ${error.message}`);
return '';
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
No description provided.