Skip to content
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

Merged
merged 26 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
2c95242
feat: e pr download-dist <number>
samuelmaddock Nov 15, 2024
49d1b02
update docs
samuelmaddock Nov 15, 2024
ce9f457
Update src/e-pr.js
samuelmaddock Nov 15, 2024
747c091
use correct executable names per platform
samuelmaddock Nov 15, 2024
88cfc67
fix guess target
samuelmaddock Nov 15, 2024
0b75495
set e pr open as default command
samuelmaddock Nov 15, 2024
b084860
allow specifying output directory
samuelmaddock Nov 15, 2024
a0afe12
fix wording
samuelmaddock Nov 15, 2024
16a510c
download to temporary directory
samuelmaddock Nov 15, 2024
9b9c07a
Update src/e
samuelmaddock Nov 15, 2024
428b4a1
download and extract in-memory
samuelmaddock Nov 15, 2024
3ad9c5c
add confirmation prompt
samuelmaddock Nov 16, 2024
50f918d
download progress
samuelmaddock Nov 17, 2024
a04453d
success color
samuelmaddock Nov 17, 2024
4cf7cb8
skip confirmation in CI
samuelmaddock Nov 17, 2024
e23dd3e
refactor: use extract-zip to extract symlinks properly
samuelmaddock Nov 21, 2024
8290315
chore: remove adm-zip
samuelmaddock Nov 21, 2024
d46dca4
fix: lockfile changes
samuelmaddock Nov 26, 2024
aef7eca
default download choice to no/false
samuelmaddock Nov 26, 2024
3eba6a7
dedupe temp file cleanup
samuelmaddock Nov 26, 2024
3ba668c
add closed pr warning
samuelmaddock Nov 26, 2024
9884569
include short commit hash in artifact dir name
samuelmaddock Dec 12, 2024
47acadb
win32 => win
samuelmaddock Dec 12, 2024
31c29c3
ehh maybe dont mix - and _
samuelmaddock Dec 12, 2024
ab93d9c
fix: remove tmp directory in case it exists
samuelmaddock Dec 12, 2024
910a85c
build: fixup yarn.lock
dsanders11 Dec 12, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
.nyc_output/
artifacts
configs
coverage
node_modules
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ git cherry-pick --continue
git push

# create pull request
e pr --backport 1234
e pr open --backport 1234
```

## Common Usage
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"command-exists": "^1.2.8",
"commander": "^9.0.0",
"debug": "^4.3.1",
"extract-zip": "^2.0.1",
Copy link
Member

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 the dist.zip file. Quick pseudo-code example:

const distZipEntry = artifactZip.getEntry('dist.zip');
if (!distZipEntry) {
  fatal(`dist.zip not found within the artifact.`);
  return;
}

const distZip = new AdmZip(artifactZip.readFile(distZipEntry));
distZip.extractAllTo(outputDir);

Copy link
Member Author

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.

Copy link
Member

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.

"inquirer": "^8.2.4",
"node-gyp": "^10.0.1",
"open": "^6.4.0",
Expand Down
22 changes: 1 addition & 21 deletions src/download.js
Original file line number Diff line number Diff line change
@@ -1,29 +1,9 @@
const fs = require('fs');
const stream = require('stream');
const { pipeline } = require('stream/promises');
const ProgressBar = require('progress');

const { fatal } = require('./utils/logging');

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;
};
const { progressStream } = require('./utils/download');

const write = fs.createWriteStream(process.argv[3]);

Expand Down
2 changes: 1 addition & 1 deletion src/e
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ program
.command('backport [pr]', 'Assists with manual backport processes')
.command('show <subcommand>', 'Show info about the current build config')
.command('test [specRunnerArgs...]', `Run Electron's spec runner`)
.command('pr [options]', 'Open a GitHub URL where you can PR your changes')
.command('pr [subcommand]', 'Work with PRs to electron/electron')
.command('patches <target>', 'Refresh the patches in $root/src/electron/patches/$target')
.command('open <sha1|PR#>', 'Open a GitHub URL for the given commit hash / pull # / issue #')
.command('auto-update', 'Check for build-tools updates or enable/disable automatic updates')
Expand Down
269 changes: 252 additions & 17 deletions src/e-pr.js
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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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',
Copy link
Member

Choose a reason for hiding this comment

The 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 win32 to win to align with GN upstream: electron/electron@510b87c), but I don't think we need to hold this PR on that change, we can update it after the e/e PR lands 🙂

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);
26 changes: 26 additions & 0 deletions src/utils/download.js
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,
};
Loading
Loading