feat: e pr download-dist <number> #679

samuelmaddock · 2024-11-15T00:22:19Z

e pr download-dist <number> will download a pull request's dist artifacts to
~/.electron_build_tools/artifacts/pr_{number}_{commithash}_{platform}_{arch}

As a followup, we could add a electron-fiddle:// command to register it as a local build for easier testing. With that, this command could register the downloaded artifact.

src/e-pr.js

itsananderson · 2024-11-15T00:49:18Z

src/e-pr.js

+    if (!(await fs.promises.stat(electronAppPath).catch(() => false))) {
+      fatal(`Electron.app not found within the extracted dist.zip.`);
+      return;
+    }


For Mac versions, do we want to automatically remove the quarantine with xattr -c Electron.app since the app will be unsigned, or do we want to leave that to the developer?

I suppose we could either prompt the caller, or add a CLI flag if it doesn't seem safe to automatically remove it

I was also wondering about this. When I run the script, and double-click the .app, it doesn't seem to show me the popup about it being downloaded from the internet 🤔

Maybe when I manually downloaded the artifact, that attribute was added by Chrome to the .zip and then propagated to the .app when I extracted the zip. If downloading it via a script doesn't encounter the same problem, then all the better :)

dsanders11

Can we implement this without it being a breaking change to e pr? I know we don't guarantee CLI stability, but it's nice to avoid breaking it in case others are using it (such as other tooling like the Electron Build Tools VS Code extension)
This won't support Windows until we migrate from AppVeyor to GHA (should be soon), might be worth calling out

samuelmaddock · 2024-11-15T04:12:05Z

Can we implement this without it being a breaking change to e pr? I know we don't guarantee CLI stability, but it's nice to avoid breaking it in case others are using it (such as other tooling like the Electron Build Tools VS Code extension)

Agreed. I was able to find a way to do this in 9e2ad26 👍

This won't support Windows until we migrate from AppVeyor to GHA (should be soon), might be worth calling out

For now this will fail when it can't the artifacts.

$ e pr download-dist 44411 --platform=win32 --arch=x64
Failed to find artifact: generated_artifacts_win32_x64

dsanders11

I'm a +1 on the general concept, but a couple of thoughts on the implementation:

Would be nice if there was a built-in way to clean out downloaded artifacts so they don't build up over time.
I think some guard rails around this would be good, to prevent a user from fat fingering a PR number and potentially running code they didn't intend to on their local machine. Perhaps a confirmation prompt that displays the title of the PR, the author, indicates if it's a fork, etc?

dsanders11 · 2024-11-15T21:58:14Z

package.json

@@ -28,6 +28,7 @@
    "command-exists": "^1.2.8",
    "commander": "^9.0.0",
    "debug": "^4.3.1",
+    "extract-zip": "^2.0.1",


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);

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.

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.

src/e

src/e-pr.js

samuelmaddock · 2024-11-16T00:12:54Z

@dsanders11 here's sample output from the confirmation prompt I added

$ e pr download-dist 44598            
? You are about to download artifacts from:

“feat: add query-session-end and improve session-end events on Windows (#44598)” by savely-krasovsky
https://github.com/savely-krasovsky/electron (fork)

Proceed? (Y/n)

$ e pr download-dist 44411            
? You are about to download artifacts from:

“feat: service worker preload scripts (#44411)” by samuelmaddock
https://github.com/electron/electron

Proceed? (Y/n)

I'm still thinking about how I want to approach cleaning up. Whether it should be another command or automatically checked each time any build-tools command is issued.

Also, excellent recommendation with extracting in-memory. It cleaned up a lot of code!

dsanders11 · 2024-11-16T00:33:50Z

yarn.lock

-"strip-ansi-cjs@npm:strip-ansi@^6.0.1":
-  version "6.0.1"
-  resolved "https://registry.yarnpkg.com/strip-ansi/-/strip-ansi-6.0.1.tgz#9e26c63d30f53443e9489495b2105d37b67a85d9"
-  integrity sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==
-  dependencies:
-    ansi-regex "^5.0.1"
-
-strip-ansi@^6.0.0, strip-ansi@^6.0.1:
+"strip-ansi-cjs@npm:strip-ansi@^6.0.1", strip-ansi@^6.0.0, strip-ansi@^6.0.1:


I think you're using an older version of Yarn so these are getting folded back into one, which can actually cause some weird issues (not sure it will in this specific case). If I revert these changes and run yarn install with v1.22.22 the only change I get is the adm-zip addition.

You're correct, I was on v1.22.17 🧠
I'll update and fix.

@samuelmaddock, once you update this I'm good to go on merging. 👍

samuelmaddock · 2024-11-16T20:52:41Z

Opened #680 for cleaning up artifacts.

samuelmaddock · 2024-11-20T01:40:09Z

Bad news regarding adm-zip, it doesn't support symlinks (cthackers/adm-zip#455) which Electron.app contains once extracted. I may have to switch back to extract-zip for this reason.

dsanders11 · 2024-11-20T07:12:44Z

@samuelmaddock, ah, unfortunate. Thanks for the update, makes sense.

dsanders11

Blocking on the lockfile change regarding wrap-ansi and friends.

As another safety mechanism I think it would be good to not allow downloading dist for closed PRs. An option could allow it if we think that's a valid use case, but if someone puts up a malicious PR I don't want it to be easy to download the artifacts after we close it.

It's also not clear to me how this PR handles changes to PRs. I think the short SHA should be worked into the artifact names to avoid confusion? There's currently no indicator which commit the artifacts are for, which seems like a recipe for getting confused about which changes are running.

src/e-pr.js

samuelmaddock · 2024-11-26T05:55:31Z

@dsanders11 how would you feel about a prompt like this regarding the closed PR issue

? You are about to download artifacts from:

“feat: service worker preload scripts (#44411)” by samuelmaddock
https://github.com/electron/electron

❗❗❗ The pull request is closed, only proceed if you trust the source ❗❗❗

Proceed? (y/N)

dsanders11 · 2024-11-26T19:30:26Z

@samuelmaddock, with that and the default being N, I think I'm good with that. 👍

VerteDinde

Agreed with the proposed changes, this looks good to me now - calling out one potential e/e change that may affect this, but given that PR hasn't merged, I don't think we need to block on it unless we'd like to wait for it to land.

VerteDinde · 2024-12-11T23:33:07Z

src/e-pr.js

+      await extractZip(distZipPath, { dir: outputDir });
+
+      const platformExecutables = {
+        win32: 'electron.exe',


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 🙂

jkleinsc

For consistency sake in electron/electron#44136, windows artifacts are going to use win instead of win32, so this PR will need to adjust the Windows artifacts names from process.platform to win.

Artifact names will be:

generated_artifacts_win_arm64
generated_artifacts_win_x64
generated_artifacts_win_x86
(eg see https://github.com/electron/electron/actions/runs/12286319546)

Co-authored-by: Will Anderson <[email protected]>

Co-authored-by: David Sanders <[email protected]>

samuelmaddock · 2024-12-12T17:14:56Z

@dsanders11 I've updated the artifact download directory to include the short commit hash (9884569):
~/.electron_build_tools/artifacts/pr_{number}_{commithash}_{platform}_{arch}

@VerteDinde @jkleinsc I've replaced win32 with win when downloading artifacts. 47acadb

samuelmaddock requested review from ckerr and a team as code owners November 15, 2024 00:22

itsananderson reviewed Nov 15, 2024

View reviewed changes

itsananderson approved these changes Nov 15, 2024

View reviewed changes

dsanders11 reviewed Nov 15, 2024

View reviewed changes

samuelmaddock mentioned this pull request Nov 15, 2024

build: use github actions for windows electron/electron#44136

Merged

samuelmaddock requested a review from dsanders11 November 15, 2024 21:02

dsanders11 reviewed Nov 15, 2024

View reviewed changes

dsanders11 reviewed Nov 16, 2024

View reviewed changes

samuelmaddock mentioned this pull request Nov 16, 2024

feat: 'e clean' for removing artifacts #680

Open

ckerr approved these changes Nov 19, 2024

View reviewed changes

samuelmaddock requested a review from dsanders11 November 21, 2024 00:12

This was referenced Nov 22, 2024

feat: register-local-version protocol command electron/fiddle#1652

Open

feat: register downloaded dist in Fiddle #682

Open

feat: service worker preload scripts electron/electron#44411

Open

dsanders11 requested changes Nov 26, 2024

View reviewed changes

src/e-pr.js Show resolved Hide resolved

src/e-pr.js Show resolved Hide resolved

src/e-pr.js Outdated Show resolved Hide resolved

VerteDinde approved these changes Dec 11, 2024

View reviewed changes

jkleinsc requested changes Dec 12, 2024

View reviewed changes

samuelmaddock and others added 5 commits December 12, 2024 11:55

feat: e pr download-dist <number>

2c95242

update docs

49d1b02

Update src/e-pr.js

ce9f457

Co-authored-by: Will Anderson <[email protected]>

use correct executable names per platform

747c091

fix guess target

88cfc67

samuelmaddock and others added 18 commits December 12, 2024 11:55

set e pr open as default command

0b75495

allow specifying output directory

b084860

fix wording

a0afe12

download to temporary directory

16a510c

Update src/e

9b9c07a

Co-authored-by: David Sanders <[email protected]>

download and extract in-memory

428b4a1

add confirmation prompt

3ad9c5c

download progress

50f918d

success color

a04453d

skip confirmation in CI

4cf7cb8

refactor: use extract-zip to extract symlinks properly

e23dd3e

chore: remove adm-zip

8290315

fix: lockfile changes

d46dca4

default download choice to no/false

aef7eca

dedupe temp file cleanup

3eba6a7

add closed pr warning

3ba668c

include short commit hash in artifact dir name

9884569

win32 => win

47acadb

samuelmaddock force-pushed the pr-download branch from 1dbbd99 to 47acadb Compare December 12, 2024 17:11

samuelmaddock added 2 commits December 12, 2024 12:30

ehh maybe dont mix - and _

31c29c3

fix: remove tmp directory in case it exists

ab93d9c

samuelmaddock requested review from jkleinsc and dsanders11 December 12, 2024 17:32

jkleinsc approved these changes Dec 12, 2024

View reviewed changes

build: fixup yarn.lock

910a85c

dsanders11 approved these changes Dec 12, 2024

View reviewed changes

samuelmaddock merged commit 4103930 into main Dec 12, 2024
9 checks passed

samuelmaddock deleted the pr-download branch December 12, 2024 23:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: e pr download-dist <number> #679

feat: e pr download-dist <number> #679

samuelmaddock commented Nov 15, 2024 •

edited

Loading

itsananderson Nov 15, 2024

samuelmaddock Nov 15, 2024

itsananderson Nov 15, 2024

dsanders11 left a comment

samuelmaddock commented Nov 15, 2024

dsanders11 left a comment

dsanders11 Nov 15, 2024

samuelmaddock Nov 15, 2024

dsanders11 Nov 15, 2024

samuelmaddock commented Nov 16, 2024 •

edited

Loading

dsanders11 Nov 16, 2024

samuelmaddock Nov 26, 2024

dsanders11 Dec 11, 2024

samuelmaddock commented Nov 16, 2024

samuelmaddock commented Nov 20, 2024

dsanders11 commented Nov 20, 2024

dsanders11 left a comment

samuelmaddock commented Nov 26, 2024

dsanders11 commented Nov 26, 2024

VerteDinde left a comment

VerteDinde Dec 11, 2024

jkleinsc left a comment •

edited

Loading

samuelmaddock commented Dec 12, 2024 •

edited

Loading

feat: e pr download-dist <number> #679

feat: e pr download-dist <number> #679

Conversation

samuelmaddock commented Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsanders11 left a comment

Choose a reason for hiding this comment

samuelmaddock commented Nov 15, 2024

dsanders11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelmaddock commented Nov 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelmaddock commented Nov 16, 2024

samuelmaddock commented Nov 20, 2024

dsanders11 commented Nov 20, 2024

dsanders11 left a comment

Choose a reason for hiding this comment

samuelmaddock commented Nov 26, 2024

dsanders11 commented Nov 26, 2024

VerteDinde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkleinsc left a comment • edited Loading

Choose a reason for hiding this comment

samuelmaddock commented Dec 12, 2024 • edited Loading

samuelmaddock commented Nov 15, 2024 •

edited

Loading

samuelmaddock commented Nov 16, 2024 •

edited

Loading

jkleinsc left a comment •

edited

Loading

samuelmaddock commented Dec 12, 2024 •

edited

Loading