Skip to content

Update electron and electron-builder#26072

Merged
gzdunek merged 7 commits intomasterfrom
gzdunek/update-electron-and-electron-builder
May 15, 2023
Merged

Update electron and electron-builder#26072
gzdunek merged 7 commits intomasterfrom
gzdunek/update-electron-and-electron-builder

Conversation

@gzdunek
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek commented May 11, 2023

Updates electron and electron-builder to the latest versions. I looked at the change lists for both and I did not see any breaking changes that would affect us.

Tested on:

  • Ubuntu 18.04 (the app does not start)
  • Ubuntu 20.04
  • Ubuntu 22.04
  • Fedora 38
  • Windows 11
  • macOS 13.3

I tested the app Ubuntu 18.04 because Node.js 18 drops support for it and I was curious if the app really would not work 🙃 (it did not).
We should update operating system support page for Connect with the requirements from Node.js 18 (I tried to find the requirements in electron docs but I could not).
The Node requirements:

  • Prebuilt binaries for Linux are now built on Red Hat Enterprise Linux (RHEL) 8 and are compatible with Linux distributions based on glibc 2.28 or later, for example, Debian 10, RHEL 8, Ubuntu 20.04.
  • Prebuilt binaries for macOS now require macOS 10.15 or later.

BTW, it looks like our page is incorrect anyway, I tried to run our current Connect v13 on Ubuntu 18.04 and the app started but it rendered only white background. The same happened to vscode.

Drone build: https://drone.platform.teleport.sh/gravitational/teleport/23758/16/2 - the build-linux-arm64 job shows as timed out, but on GHA it succeeded after 3.5 hours. Anyway, it is not related to Connect.

I also removed electron-debug - we do not use it.

While testing the app on the virtual machines I experienced a lot of rendering artifacts (like suddenly disappearing background of the login modal or some weird green, green or pink lines and triangles). I do not think it is a new thing and it is related to a virtual machine, but I would be grateful @avatus if you could try it on some real Linux 🙏 (if you still have one).

Copy link
Copy Markdown
Contributor

@avatus avatus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works on Ubuntu 22.04 👍

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from ravicious May 11, 2023 16:26
@ravicious ravicious self-requested a review May 12, 2023 13:34
@ravicious
Copy link
Copy Markdown
Member

I tried to find the requirements in electron docs but I could not

They're in the readme on GitHub, but they appear to be out of date.

24.3.0 came out 2 days ago, I'm running a tag build to verify if it builds correctly. If it does, could you bump the version in this PR?

Also, could you update after-install and after-remove scripts? I thought I added that as an item to the electron-builder update checklist but it turns out that I didn't. I'll add them now.

@gzdunek
Copy link
Copy Markdown
Contributor Author

gzdunek commented May 15, 2023

24.3.0 came out 2 days ago, I'm running a tag build to verify if it builds correctly. If it does, could you bump the version in this PR?

Done.

Also, could you update after-install and after-remove scripts? I thought I added that as an item to the electron-builder update checklist but it turns out that I didn't. I'll add them now.

Done. I tested it on Ubuntu, the symlinks adding/removal seem to work fine.
While removing the app I got:

Removing teleport-connect (1.0.0-dev) ...
update-alternatives: warning: alternative /opt/Teleport Connect/teleport-connect (part of link group teleport-connect) doesn't exist; removing from list of alternatives
update-alternatives: warning: /etc/alternatives/teleport-connect is dangling; it will be updated with best choice

but I guess it is a standard message. The symlink was removed.

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from ravicious May 15, 2023 12:40
@gzdunek gzdunek requested a review from ravicious May 15, 2023 12:52
@gzdunek gzdunek force-pushed the gzdunek/update-electron-and-electron-builder branch from f15e33e to 76a9d97 Compare May 15, 2023 13:06
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from ravicious May 15, 2023 13:07
Copy link
Copy Markdown
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the install scripts on Ubuntu and Fedora. I also tested the universal build on arm64 and x64.

# Remove the link to the Electron app binary.
rm -f "$BIN/${executable}"
if type update-alternatives >/dev/null 2>&1; then
update-alternatives --remove "${executable}" "$BIN/${executable}"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that the call to update-alternatives in after-remove appears to be incorrect in electron-builder's template.

On Fedora, installation works without problems, but on remove I got this:

/usr/local/bin/teleport-connect has not been configured as an alternative for teleport-connect
warning: %postun(teleport-connect-1.0.0_dev-1.aarch64) scriptlet failed, exit status 2

I tried to execute the commands manually and I got some other error, then it occured to me that the call might be wrong.

$ update-alternatives --help
alternatives version 1.24 - Copyright (C) 2001 Red Hat, Inc.
This may be freely redistributed under the terms of the GNU Public License.

usage: alternatives --install <link> <name> <path> <priority>
                    [--initscript <service>]
                    [--family <family>]
                    [--follower <follower_link> <follower_name> <follower_path>]*
       alternatives --remove <name> <path>
Suggested change
update-alternatives --remove "${executable}" "$BIN/${executable}"
update-alternatives --remove "${executable}" "$APP/${executable}"

(but you also need to copy APP from after-install and define it in after-remove)

After applying this change, the error on Fedora is gone. The warning on Ubuntu still persists though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have tested it on Fedora 😞 My only excuse is that I was slightly tired of testing the app before opening the PR.

Anyway, good catch! WDYT about opening a PR with the fix in electron-builder?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean if you want to then go for it. 😏

@gzdunek gzdunek added this pull request to the merge queue May 15, 2023
Merged via the queue into master with commit a5679de May 15, 2023
@gzdunek gzdunek deleted the gzdunek/update-electron-and-electron-builder branch May 15, 2023 17:07
@public-teleport-github-review-bot
Copy link
Copy Markdown

@gzdunek See the table below for backport results.

Branch Result
branch/v11 Failed
branch/v12 Failed
branch/v13 Create PR

gzdunek added a commit that referenced this pull request May 16, 2023
* Upgrade electron and electron-builder

* Update the path to x64 build

* Update electron to 24.3.0

* Use the latest after-install and after-remove scripts

* Run prettier

* Use correct indentation

* Use app path in `update-alternatives --remove`

(cherry picked from commit a5679de)
gzdunek added a commit that referenced this pull request May 16, 2023
* Upgrade electron and electron-builder

* Update the path to x64 build

* Update electron to 24.3.0

* Use the latest after-install and after-remove scripts

* Run prettier

* Use correct indentation

* Use app path in `update-alternatives --remove`

(cherry picked from commit a5679de)
stevenGravy pushed a commit that referenced this pull request May 17, 2023
* Upgrade electron and electron-builder

* Update the path to x64 build

* Update electron to 24.3.0

* Use the latest after-install and after-remove scripts

* Run prettier

* Use correct indentation

* Use app path in `update-alternatives --remove`

(cherry picked from commit a5679de)
gzdunek added a commit that referenced this pull request May 17, 2023
* Update `electron` and `electron-builder` (#26072)

* Upgrade electron and electron-builder

* Update the path to x64 build

* Update electron to 24.3.0

* Use the latest after-install and after-remove scripts

* Run prettier

* Use correct indentation

* Use app path in `update-alternatives --remove`

(cherry picked from commit a5679de)

* Update yarn.lock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants