-
Notifications
You must be signed in to change notification settings - Fork 77
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: e pr download-dist <number> #679
Changes from all commits
2c95242
49d1b02
ce9f457
747c091
88cfc67
0b75495
b084860
a0afe12
16a510c
9b9c07a
428b4a1
3ad9c5c
50f918d
a04453d
4cf7cb8
e23dd3e
8290315
d46dca4
aef7eca
3eba6a7
3ba668c
9884569
47acadb
31c29c3
ab93d9c
910a85c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
.nyc_output/ | ||
artifacts | ||
configs | ||
coverage | ||
node_modules | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,26 @@ | ||
#!/usr/bin/env node | ||
|
||
const childProcess = require('child_process'); | ||
const fs = require('fs'); | ||
const os = require('os'); | ||
const path = require('path'); | ||
const { Readable } = require('stream'); | ||
const { pipeline } = require('stream/promises'); | ||
|
||
const extractZip = require('extract-zip'); | ||
const querystring = require('querystring'); | ||
const semver = require('semver'); | ||
|
||
const open = require('open'); | ||
const program = require('commander'); | ||
const { Octokit } = require('@octokit/rest'); | ||
const inquirer = require('inquirer'); | ||
|
||
const { progressStream } = require('./utils/download'); | ||
const { getGitHubAuthToken } = require('./utils/github-auth'); | ||
const { current } = require('./evm-config'); | ||
const { color, fatal } = require('./utils/logging'); | ||
const { color, fatal, logError } = require('./utils/logging'); | ||
|
||
const d = require('debug')('build-tools:pr'); | ||
|
||
// Adapted from https://github.com/electron/clerk | ||
function findNoteInPRBody(body) { | ||
|
@@ -134,27 +145,23 @@ function pullRequestSource(source) { | |
} | ||
|
||
program | ||
.command('open', null, { isDefault: true }) | ||
.description('Open a GitHub URL where you can PR your changes') | ||
.option( | ||
'-s, --source <source_branch>', | ||
'Where the changes are coming from', | ||
guessPRSource(current()), | ||
) | ||
.option( | ||
'-t, --target <target_branch>', | ||
'Where the changes are going to', | ||
guessPRTarget(current()), | ||
) | ||
.option('-s, --source [source_branch]', 'Where the changes are coming from') | ||
.option('-t, --target [target_branch]', 'Where the changes are going to') | ||
.option('-b, --backport <pull_request>', 'Pull request being backported') | ||
.action(async (options) => { | ||
if (!options.source) { | ||
const source = options.source || guessPRSource(current()); | ||
const target = options.target || guessPRTarget(current()); | ||
dsanders11 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (!source) { | ||
fatal(`'source' is required to create a PR`); | ||
} else if (!options.target) { | ||
} else if (!target) { | ||
fatal(`'target' is required to create a PR`); | ||
} | ||
|
||
const repoBaseUrl = 'https://github.com/electron/electron'; | ||
const comparePath = `${options.target}...${pullRequestSource(options.source)}`; | ||
const comparePath = `${target}...${pullRequestSource(source)}`; | ||
const queryParams = { expand: 1 }; | ||
|
||
if (!options.backport) { | ||
|
@@ -188,5 +195,233 @@ program | |
} | ||
|
||
return open(`${repoBaseUrl}/compare/${comparePath}?${querystring.stringify(queryParams)}`); | ||
}) | ||
.parse(process.argv); | ||
}); | ||
|
||
program | ||
.command('download-dist <pull_request_number>') | ||
.description('Download a pull request dist') | ||
.option( | ||
'--platform [platform]', | ||
'Platform to download dist for. Defaults to current platform.', | ||
process.platform, | ||
) | ||
.option( | ||
'--arch [arch]', | ||
'Architecture to download dist for. Defaults to current arch.', | ||
process.arch, | ||
) | ||
.option( | ||
'-o, --output <output_directory>', | ||
'Specify the output directory for downloaded artifacts. ' + | ||
'Defaults to ~/.electron_build_tools/artifacts/pr_{number}_{commithash}_{platform}_{arch}', | ||
) | ||
.option( | ||
'-s, --skip-confirmation', | ||
'Skip the confirmation prompt before downloading the dist.', | ||
!!process.env.CI, | ||
) | ||
.action(async (pullRequestNumber, options) => { | ||
if (!pullRequestNumber) { | ||
fatal(`Pull request number is required to download a PR`); | ||
} | ||
|
||
d('checking auth...'); | ||
const auth = await getGitHubAuthToken(['repo']); | ||
const octokit = new Octokit({ auth }); | ||
|
||
d('fetching pr info...'); | ||
let pullRequest; | ||
try { | ||
const { data } = await octokit.pulls.get({ | ||
owner: 'electron', | ||
repo: 'electron', | ||
pull_number: pullRequestNumber, | ||
}); | ||
pullRequest = data; | ||
} catch (error) { | ||
console.error(`Failed to get pull request: ${error}`); | ||
return; | ||
} | ||
|
||
if (!options.skipConfirmation) { | ||
const isElectronRepo = pullRequest.head.repo.full_name !== 'electron/electron'; | ||
const { proceed } = await inquirer.prompt([ | ||
{ | ||
type: 'confirm', | ||
default: false, | ||
name: 'proceed', | ||
message: `You are about to download artifacts from: | ||
|
||
“${pullRequest.title} (#${pullRequest.number})” by ${pullRequest.user.login} | ||
${pullRequest.head.repo.html_url}${isElectronRepo ? ' (fork)' : ''} | ||
${pullRequest.state !== 'open' ? '\n❗❗❗ The pull request is closed, only proceed if you trust the source ❗❗❗\n' : ''} | ||
Proceed?`, | ||
}, | ||
]); | ||
|
||
if (!proceed) return; | ||
} | ||
|
||
d('fetching workflow runs...'); | ||
let workflowRuns; | ||
try { | ||
const { data } = await octokit.actions.listWorkflowRunsForRepo({ | ||
owner: 'electron', | ||
repo: 'electron', | ||
branch: pullRequest.head.ref, | ||
name: 'Build', | ||
event: 'pull_request', | ||
status: 'completed', | ||
per_page: 10, | ||
sort: 'created', | ||
direction: 'desc', | ||
}); | ||
dsanders11 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
workflowRuns = data.workflow_runs; | ||
} catch (error) { | ||
console.error(`Failed to list workflow runs: ${error}`); | ||
return; | ||
} | ||
|
||
const latestBuildWorkflowRun = workflowRuns.find((run) => run.name === 'Build'); | ||
if (!latestBuildWorkflowRun) { | ||
fatal(`No 'Build' workflow runs found for pull request #${pullRequestNumber}`); | ||
} | ||
const shortCommitHash = latestBuildWorkflowRun.head_sha.substring(0, 7); | ||
|
||
d('fetching artifacts...'); | ||
let artifacts; | ||
try { | ||
const { data } = await octokit.actions.listWorkflowRunArtifacts({ | ||
owner: 'electron', | ||
repo: 'electron', | ||
run_id: latestBuildWorkflowRun.id, | ||
}); | ||
artifacts = data.artifacts; | ||
} catch (error) { | ||
console.error(`Failed to list artifacts: ${error}`); | ||
return; | ||
} | ||
|
||
const artifactPlatform = options.platform === 'win32' ? 'win' : options.platform; | ||
const artifactName = `generated_artifacts_${artifactPlatform}_${options.arch}`; | ||
const artifact = artifacts.find((artifact) => artifact.name === artifactName); | ||
if (!artifact) { | ||
console.error(`Failed to find artifact: ${artifactName}`); | ||
return; | ||
} | ||
|
||
let outputDir; | ||
|
||
if (options.output) { | ||
outputDir = path.resolve(options.output); | ||
|
||
if (!(await fs.promises.stat(outputDir).catch(() => false))) { | ||
fatal(`The output directory '${options.output}' does not exist`); | ||
} | ||
} else { | ||
const artifactsDir = path.resolve(__dirname, '..', 'artifacts'); | ||
const defaultDir = path.resolve( | ||
artifactsDir, | ||
`pr_${pullRequest.number}_${shortCommitHash}_${options.platform}_${options.arch}`, | ||
); | ||
|
||
// Clean up the directory if it exists | ||
try { | ||
await fs.promises.rm(defaultDir, { recursive: true, force: true }); | ||
} catch (error) { | ||
if (error.code !== 'ENOENT') { | ||
throw error; | ||
} | ||
} | ||
|
||
// Create the directory | ||
await fs.promises.mkdir(defaultDir, { recursive: true }); | ||
|
||
outputDir = defaultDir; | ||
} | ||
|
||
console.log( | ||
`Downloading artifact '${artifactName}' from pull request #${pullRequestNumber}...`, | ||
); | ||
|
||
// Download the artifact to a temporary directory | ||
const tempDir = path.join(os.tmpdir(), 'electron-tmp'); | ||
await fs.promises.rm(tempDir, { recursive: true, force: true }); | ||
await fs.promises.mkdir(tempDir); | ||
|
||
const { url } = await octokit.actions.downloadArtifact.endpoint({ | ||
owner: 'electron', | ||
repo: 'electron', | ||
artifact_id: artifact.id, | ||
archive_format: 'zip', | ||
}); | ||
|
||
const response = await fetch(url, { | ||
headers: { | ||
Authorization: `Bearer ${auth}`, | ||
}, | ||
}); | ||
|
||
if (!response.ok) { | ||
fatal(`Could not find artifact: ${url} got ${response.status}`); | ||
} | ||
|
||
const total = parseInt(response.headers.get('content-length'), 10); | ||
const artifactDownloadStream = Readable.fromWeb(response.body); | ||
|
||
try { | ||
const artifactZipPath = path.join(tempDir, `${artifactName}.zip`); | ||
const artifactFileStream = fs.createWriteStream(artifactZipPath); | ||
await pipeline( | ||
artifactDownloadStream, | ||
// Show download progress | ||
...(process.env.CI ? [] : [progressStream(total, '[:bar] :mbRateMB/s :percent :etas')]), | ||
artifactFileStream, | ||
); | ||
|
||
// Extract artifact zip | ||
d('unzipping artifact to %s', tempDir); | ||
await extractZip(artifactZipPath, { dir: tempDir }); | ||
|
||
// Check if dist.zip exists within the extracted artifact | ||
const distZipPath = path.join(tempDir, 'dist.zip'); | ||
if (!(await fs.promises.stat(distZipPath).catch(() => false))) { | ||
throw new Error(`dist.zip not found in build artifact.`); | ||
} | ||
|
||
// Extract dist.zip | ||
// NOTE: 'extract-zip' is used as it correctly extracts symlinks. | ||
d('unzipping dist.zip to %s', outputDir); | ||
await extractZip(distZipPath, { dir: outputDir }); | ||
|
||
const platformExecutables = { | ||
win32: 'electron.exe', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a heads up, we're looking at a PR in e/e right now to move Windows CI over to GitHub actions that might break this (I think we're changing |
||
darwin: 'Electron.app/', | ||
linux: 'electron', | ||
}; | ||
|
||
const executableName = platformExecutables[options.platform]; | ||
if (!executableName) { | ||
throw new Error(`Unable to find executable for platform '${options.platform}'`); | ||
} | ||
|
||
const executablePath = path.join(outputDir, executableName); | ||
if (!(await fs.promises.stat(executablePath).catch(() => false))) { | ||
throw new Error(`${executableName} not found within dist.zip.`); | ||
} | ||
|
||
console.log(`${color.success} Downloaded to ${outputDir}`); | ||
} catch (error) { | ||
logError(error); | ||
process.exitCode = 1; // wait for cleanup | ||
} finally { | ||
// Cleanup temporary files | ||
try { | ||
await fs.promises.rm(tempDir, { recursive: true }); | ||
} catch { | ||
// ignore | ||
} | ||
samuelmaddock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
}); | ||
|
||
program.parse(process.argv); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
const stream = require('stream'); | ||
const ProgressBar = require('progress'); | ||
|
||
const MB_BYTES = 1024 * 1024; | ||
|
||
const progressStream = function (total, tokens) { | ||
var pt = new stream.PassThrough(); | ||
|
||
pt.on('pipe', function (_stream) { | ||
const bar = new ProgressBar(tokens, { total: Math.round(total) }); | ||
|
||
pt.on('data', function (chunk) { | ||
const elapsed = new Date() - bar.start; | ||
const rate = bar.curr / (elapsed / 1000); | ||
bar.tick(chunk.length, { | ||
mbRate: (rate / MB_BYTES).toFixed(2), | ||
}); | ||
}); | ||
}); | ||
|
||
return pt; | ||
}; | ||
|
||
module.exports = { | ||
progressStream, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract-zip
is unmaintained and the last release was 4 years ago, so I'd prefer to use a maintained alternative if we're adding a new dependency.IMO https://www.npmjs.com/package/adm-zip is a solid alternative, and it can extract to a
Buffer
in-memory which could be useful to avoid the temp dir logic (at the expense of higher memory usage) and get straight to thedist.zip
file. Quick pseudo-code example:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is validating actually 👍 I would have reached for that package first, but saw we were using
extract-zip
in many other places so I was aiming for consistency.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, historically we've used it but probably time to move on given it's unmaintained. I have loose plans to migrate our other usages of it as appropriate.