chore(deps): bump the electron group across 1 directory with 5 updates#62704
chore(deps): bump the electron group across 1 directory with 5 updates#62704dependabot[bot] wants to merge 7 commits intomasterfrom
Conversation
--- updated-dependencies: - dependency-name: node-pty dependency-version: 1.1.0 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: electron - dependency-name: electron dependency-version: 39.2.7 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: electron - dependency-name: electron-builder dependency-version: 26.4.0 dependency-type: direct:development update-type: version-update:semver-minor dependency-group: electron - dependency-name: electron-updater dependency-version: 6.7.3 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: electron - dependency-name: electron-vite dependency-version: 5.0.0 dependency-type: direct:development update-type: version-update:semver-major dependency-group: electron ... Signed-off-by: dependabot[bot] <support@github.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| "@types/which": "^3.0.4", | ||
| "node-forge": "^1.3.3", | ||
| "node-pty": "1.1.0-beta35", | ||
| "node-pty": "1.1.0", |
There was a problem hiding this comment.
1.1.0 is still susceptible to the dev build error on macOS unfortunately.
The fix for it (microsoft/node-pty#850) has been shipped in 1.2.0-beta.2. I think we should just update to it since we've been using a beta version of this package all this time anyway.
There was a problem hiding this comment.
With 1.2.0-beta.2, the build now fails with this:
⨯ Detected file "Contents/Resources/app.asar.unpacked/node_modules/node-pty/prebuilds/darwin-arm64/pty.node" that's the same in both x64 and arm64 builds and not covered by the x64ArchFiles rule: "Contents/MacOS/tsh.app/Contents/MacOS/tsh"
atmakeUniversalApp(/Users/runner/work/teleport.e/teleport.e/node_modules/.pnpm/@electron+universal@2.0.3/node_modules/@electron/universal/src/index.ts:181:17)
It looks like since some version of node-pty that's > 1.1.0-beta35, the node-pty package ships with a prebuilds directory. You can see it here: https://www.npmjs.com/package/node-pty/v/1.2.0-beta.2?activeTab=code
I created an issue in node-pty about this as I'm not sure if @electron/universal is supposed to ignore those files or not. microsoft/node-pty#863
There was a problem hiding this comment.
This is how the system works currently. I recommend deleting the binaries that aren't relevant in the meantime. I'd like us to ship per-arch modules eventually.
@gzdunek Do you know off the top of your head how complex this would be? Because I'm weighing whether we should update or stay on 1.1.0-beta35 for a while.
I imagine it's going to include one of the build hooks and an annoying back-and-forth until we figure out which one runs at the correct stage of packaging the app. Though maybe it'd be a matter of adding console.log to all of them and then figuring out how to remove the unnecessary binaries.
There was a problem hiding this comment.
Detected file "Contents/Resources/app.asar.unpacked/node_modules/node-pty/prebuilds/darwin-arm64/pty.node" that's the same in both x64 and arm64 builds and not covered by the x64ArchFiles rule: "Contents/MacOS/tsh.app/Contents/MacOS/tsh"
I'm not 100% sure, but I guess we should add Contents/Resources/app.asar.unpacked/node_modules/node-pty/prebuilds/** to x64ArchFiles, instead of deleting the binaries.
I'm also worried if it means that the size of the app that includes node-pty is going to be increased by 60 MB of prebuilt binaries from win32 directories
This should probably be ignored like previously:
https://github.com/gravitational/teleport/pull/39296/changes#diff-9fa1aae9168524bdd0a97987f55e4a7ca8b77cd8c8cb47b5cfc33eb4e2a75230
Overall, it shouldn't be too complex.
There was a problem hiding this comment.
Ah, I forgot that the issue with universal builds is present only for macOS builds. It indeed might not bee that complex.
There was a problem hiding this comment.
Ok, I added this pattern to x64ArchFiles and now pnpm package-term --universal seems to pass. I wasn't able to verify if the app works because it fails with another error.
The node-pty dir in app.asar.unpacked is only 3.2 MB, so I didn't bother removing any extra stuff (though we most probably could slim the size down by removing it).
There was a problem hiding this comment.
|
Electron 40 (which includes Node 24) has just been released, but we're not able to upgrade yet due to the |
| "electron-builder": "^26.0.12", | ||
| "electron-updater": "^6.7.0", | ||
| "electron-vite": "^4.0.1", | ||
| "electron": "39.2.7", |
There was a problem hiding this comment.
There's a workaround for the issue with the windows not showing up on Linux after launch. electron/electron#48859 (comment) We should probably add it before merging 38+ to v18.
v18 is still on v37 (which we should update to the newest v37; it's already EOL but at least v18 will be on the newest possible version before we backport Go 1.25 and change macOS requirements).
| "electron-updater": "^6.7.0", | ||
| "electron-vite": "^4.0.1", | ||
| "electron": "39.2.7", | ||
| "electron-builder": "^26.4.0", |
There was a problem hiding this comment.
The tar vulnerability (https://github.com/gravitational/teleport/security/dependabot/519) can be addressed once electron-userland/electron-builder#9518 is closed. I guess we'll also have to try to backport that electron-builder update to release branches. I don't know if electron-builder and its deps are indeed vulnerable, but that won't stop bots from flagging this down.
| "electron-builder": "^26.0.12", | ||
| "electron-updater": "^6.7.0", | ||
| "electron-vite": "^4.0.1", | ||
| "electron": "39.2.7", |
There was a problem hiding this comment.
Now when I run the universal build made locally on my machine, I get this error:
Termination Reason: Namespace DYLD, Code 1, Library missing
Library not loaded: @rpath/Electron Framework.framework/Electron Framework
Referenced from: <4C4C44EF-5555-3144-A18B-1553BBC56316> /Users/USER/*/Teleport Connect.app/Contents/MacOS/Teleport Connect
Reason: tried: '/Users/rav/src/teleport/web/packages/teleterm/build/release/mac-universal/Teleport Connect.app/Contents/Frameworks/Electron Framework.framework/Electron Framework' (code signature in <4C4C4435-5555-3144-A1C4-7C0945B42800> '/Users/rav/src/teleport/web/packages/teleterm/build/release/mac-universal/Teleport Connect.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework' not valid for use in process: mapping process and mapped file (non-platform) have different Team IDs), '/Users/rav/src/teleport/web/packages/teleterm/build/release/mac-universal/Teleport Connect.app/Contents/Frameworks/Electron Framework.framework/Electron Framework' (code signature in <4C4C4435-5555-3144-A1C4-7C0945B42800> '/Users/rav/src/telep
(terminated at launch; ignore backtrace)
I haven't looked at it yet though, maybe it's normal that the universal build doesn't work when done locally?
There was a problem hiding this comment.
The change to x64ArchFiles breaks the universal build :/
But I have no idea what's wrong with it.
EDIT: Sorry, it's not related to x64ArchFiles.
There was a problem hiding this comment.
It's caused by electron-builder update, the last working version that doesn't show this error is 26.0.12.
Probably related to electron-userland/electron-builder#9007.
There was a problem hiding this comment.
electron-userland/electron-builder#5850 (comment)
The reason you didn't get this in 26.0.12 is that the hardened runtime on/off state is stored in the code signature. When there's no code signature, hardened runtime defaults to off and this check doesn't happen.
I guess this tracks with what I saw in codesign output between the current and updated version. #62704 (comment) It seems that the current version doesn't sign the app at all whereas the updated version uses ad-hoc signing.
https://wiki.lazarus.freepascal.org/Code_Signing_for_macOS#Ad_hoc_signing
There was a problem hiding this comment.
It seems the right fix would be to provide different entitlements for ad-hoc signing. But how to detect it? Maybe check if these var are not defined?
teleport/web/packages/teleterm/electron-builder-config.js
Lines 46 to 54 in 0a78c81
There was a problem hiding this comment.
Yeah. I wish we didn't have to do this as this introduces a risk of shipping the app with wrong entitlements. But that seems to be the best way for now.
I added the entitlements but something is wrong. The code seems to pick the right file but in the end codesign -d --entitlements :- web/packages/teleterm/build/release/mac-universal/Teleport\ Connect.app reports only allow-jit and I get the same error when attempting to launch the app. I haven't had the time to debug this yet.
There was a problem hiding this comment.
It's because this condition applies only to entitlementsInherit, but there's also a separate entitlements:
The path to entitlements file for signing the app. build/entitlements.mac.plist will be used if exists (it is a recommended way to set).
We need to define it explicitly, I've pushed a fix.
|
Looks like these dependencies are updatable in another way, so this is no longer needed. |
Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot mergewill merge this PR after your CI passes on it@dependabot squash and mergewill squash and merge this PR after your CI passes on it@dependabot cancel mergewill cancel a previously requested merge and block automerging@dependabot reopenwill reopen this PR if it is closed@dependabot closewill close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot show <dependency name> ignore conditionswill show all of the ignore conditions of the specified dependency@dependabot ignore <dependency name> major versionwill close this group update PR and stop Dependabot creating any more for the specific dependency's major version (unless you unignore this specific dependency's major version or upgrade to it yourself)@dependabot ignore <dependency name> minor versionwill close this group update PR and stop Dependabot creating any more for the specific dependency's minor version (unless you unignore this specific dependency's minor version or upgrade to it yourself)@dependabot ignore <dependency name>will close this group update PR and stop Dependabot creating any more for the specific dependency (unless you unignore this specific dependency or upgrade to it yourself)@dependabot unignore <dependency name>will remove all of the ignore conditions of the specified dependency@dependabot unignore <dependency name> <ignore condition>will remove the ignore condition of the specified dependency and ignore conditions