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: support AppImage releases #1379

Merged
merged 3 commits into from
Jul 3, 2023
Merged

feat: support AppImage releases #1379

merged 3 commits into from
Jul 3, 2023

Conversation

robertgzr
Copy link
Contributor

using @reforged/maker-appimage

Closes: #405
Signed-off-by: Robert Günzler r@gnzler.io

@robertgzr
Copy link
Contributor Author

error @reforged/maker-appimage@3.3.0: The engine "node" is incompatible with this module. Expected version ">=19.0.0 || ^18.11.0 || ^16.1.0". Got "18.4.0"

😓

@erickzhao
Copy link
Member

@robertgzr I think it would be fine to upgrade the Node.js version to the latest version of 18 LTS here:

- node/install:
node-version: '18.4.0'
- run: nvm use 18.4.0

@robertgzr robertgzr marked this pull request as ready for review June 28, 2023 08:18
@robertgzr robertgzr requested review from a team and codebytere as code owners June 28, 2023 08:18
Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

Few things here.

  • I think this should be feat:, not fix:, as it's adding a feature rather than fixing a bug (also minor nit, but lowercase Support would better match existing commit style 🙂)
  • We could use just 18.16 for the changes to the CircleCI config, to ensure it uses the latest fix release
  • Interestingly Node.js v18.16.1 seems to cause test failures (repros locally), so I'm investigating
  • The updates to yarn.lock here cause a bit of a split brain since it uses newer @electron-forge versions than the rest. Ideally it would keep everything on 6.1.1 - I don't have an immediate solution off the top of my head

@dsanders11
Copy link
Member

Looks like the test failures with Node.js v18.16.x are due to nodejs/node#47563, the fix for which may land soon in v18.16.2 - in the mean time let's use 18.15 for this PR, since that line doesn't have the bug (confirmed locally, and it's mentioned in the last comment on that issue).

yarn.lock Outdated Show resolved Hide resolved
@robertgzr robertgzr force-pushed the main branch 2 times, most recently from 0895cb6 to 84ba574 Compare June 28, 2023 17:40
@dsanders11
Copy link
Member

dsanders11 commented Jun 28, 2023

We could use just 18.16 for the changes to the CircleCI config, to ensure it uses the latest fix release

Ugh, this is my bad - I forgot there's an issue where nvm-windows doesn't handle versions like that, only the full one - we should add a comment in the CircleCI config. So this should use 18.15.0. Apologies for the churn.

@dsanders11 dsanders11 changed the title fix: Support AppImage releases feat: support AppImage releases Jun 28, 2023
@robertgzr
Copy link
Contributor Author

Ugh, this is my bad - I forgot there's an issue where nvm-windows doesn't handle versions like that, only the full one - we should add a comment in the CircleCI config. So this should use 18.15.0. Apologies for the churn.

looks like coreybutler/nvm-windows#709 (in 1.1.10) fixes that...

we get nvm from the windows orb, right? how would we know which version that is on 😅

@dsanders11
Copy link
Member

we get nvm from the windows orb, right? how would we know which version that is on 😅

That comes from the CircleCI images where they bake new ones every once in a while, looks like they baked a new Windows Server 2022 one in May 2023 - win/default defaults to win/server-2022 currently. Clicking through that forum post to the commit, looks like this is the file which defines the versions of dev tools, including nvm-windows, and it's currently on 1.1.9. 😅

using @reforged/maker-appimage

Closes: electron#405
Signed-off-by: Robert Günzler <r@gnzler.io>
Signed-off-by: Robert Günzler <r@gnzler.io>
avoid LTS for now until nodejs/node#47563
lands (probably 18.16.2)

Signed-off-by: Robert Günzler <r@gnzler.io>
@robertgzr
Copy link
Contributor Author

I included a comment mentioning the nvm-windows situation and linking to that ansible file :)

@codebytere codebytere merged commit 7e04e5b into electron:main Jul 3, 2023
8 checks passed
@dsanders11
Copy link
Member

@robertgzr, now that this has shipped, a couple of observations:

  • We should get the icon working properly - I played with it for a bit but wasn't successful
  • The AppImage release doesn't work with launching Fiddle from the protocol handler via the docs

@robertgzr
Copy link
Contributor Author

AppImage release doesn't work with launching Fiddle from the protocol handler via the docs

this requires some action on the system that wants this to work: https://docs.appimage.org/user-guide/run-appimages.html#integrating-appimages-into-the-desktop

it comes down to making the desktop file visible to the host, so it can detect the mimetype field and register fiddle as the handler for it, see https://manpages.debian.org/bookworm/desktop-file-utils/update-desktop-database.1.en.html

i guess we could document this?

@dsanders11
Copy link
Member

I included a comment mentioning the nvm-windows situation and linking to that ansible file :)

I poked CircleCI and they've updated nvm-windows on the latest Windows Server 2022 image, but it hasn't made it's way to being the current tag yet. Confirmed it resolves the issue: https://app.circleci.com/pipelines/github/electron/fiddle/991/workflows/5321ebad-4d4d-4ce3-bb65-fa45ec243649/jobs/3594

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support appimage releases
4 participants