fix: Use `update-alternatives` instead of symlinks for #7500 #7501

markizano · 2023-03-25T19:07:22Z

Where possible, use update-alternatives instead to avoid hardcoding links in Linux. This will allow for downstream users to specify paths to their own executables with higher priority if they wish.

Backwards compatibility is preserved by still using the symlinking route if the command update-alternatives is not available.

…inks to executable if available. Where possible, use `update-alternatives` instead to avoid hardcoding links in Linux. This will allow for downstream users to specify paths to their own executables with higher priority if they wish. Backwards compatibility is preserved by still using the symlinking route if the command `update-alternatives` is not available.

changeset-bot · 2023-03-25T19:07:25Z

🦋 Changeset detected

Latest commit: ea5c970

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages

Name	Type
app-builder-lib	Patch
dmg-builder	Patch
electron-builder-squirrel-windows	Patch
electron-builder	Patch
electron-forge-maker-appimage	Patch
electron-forge-maker-nsis-web	Patch
electron-forge-maker-nsis	Patch
electron-forge-maker-snap	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify · 2023-03-25T19:07:28Z

✅ Deploy Preview for car-park-attendant-cleat-11576 ready!

Name	Link
🔨 Latest commit	`ea5c970`
🔍 Latest deploy log	https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/6430452c5c8fe40008781dbe
😎 Deploy Preview	https://deploy-preview-7501--car-park-attendant-cleat-11576.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

markizano · 2023-03-25T21:05:19Z

okay, I think I have included all the requirements for this to be accepted.

Ready for review!

mmaietta · 2023-03-26T18:14:10Z

Thank you!
Test is failing

  ● executable path in postinst script

    expect(received).toMatchSnapshot()

    Snapshot name: `executable path in postinst script 5`

    - Snapshot  - 1
    + Received  + 5

    @@ -1,9 +1,13 @@
      #!/bin/bash

      # Link to the binary
    - ln -sf '/opt/foo/Boo' '/usr/bin/Boo'
    + if type update-alternatives 2>/dev/null >&1; then
    +    update-alternatives --install '/usr/bin/Boo' 'Boo' '/opt/foo/Boo' 100
    + else
    + ln -sf '/opt/foo/Boo' '/usr/bin/Boo'
    + fi

You'll need to run the specific test file and update the snapshot

TESt_FILES= debTest UPDATE_SNAPSHOT=true pnpm test

(Might need to run within docker image, not sure off the top of my head

markizano · 2023-04-04T23:06:24Z

Seems like I am missing post-compiled resources?

When running this command, I get errors around a missing compiled test file:

redacted@redacted:~/git/signalapp/electron-builder $ docker run --rm -e DEBUG=${DEBUG:-} -e UPDATE_SNAPSHOT=true -e TEST_FILES="${TEST_FILES:-HoistedNodeModuleTest}" -v $(pwd):/project -v $(pwd)-node-modules:/project/node_modules -v $HOME/Library/Caches/electron:/root/.cache/electron -v $HOME/Library/Caches/electron-builder:/root/.cache/electron-builder electronuserland/builder:wine-mono /bin/bash -c "pnpm install && node ./test/out/helpers/runTests.js"
Scope: all 14 workspace projects
 WARN  There are cyclic workspace dependencies: /project/packages/app-builder-lib, /project/packages/dmg-builder; /project/packages/app-builder-lib, /project/packages/dmg-builder, /project/packages/app-builder-lib, /project/packages/electron-builder-squirrel-windows
Lockfile is up to date, resolution step is skipped
Progress: resolved 1, reused 0, downloaded 0, added 0
Packages: +10 -1
++++++++++-

   ╭─────────────────────────────────────────────────────────────────╮
   │                                                                 │
   │                Update available! 7.13.2 → 8.1.1.                │
   │   Changelog: https://github.com/pnpm/pnpm/releases/tag/v8.1.1   │
   │                Run "pnpm add -g pnpm" to update.                │
   │                                                                 │
   │     Follow @pnpmjs for updates: https://twitter.com/pnpmjs      │
   │                                                                 │
   ╰─────────────────────────────────────────────────────────────────╯


dependencies:
+ dmg-license 1.0.11
- pnpm 7.30.3

. prepare$ husky install
. prepare: fatal: detected dubious ownership in repository at '/project'
. prepare: To add an exception for this directory, call:
. prepare: 	git config --global --add safe.directory /project
. prepare: Done
Progress: resolved 10, reused 8, downloaded 2, added 10, done
internal/modules/cjs/loader.js:905
  throw err;
  ^

Error: Cannot find module '/project/test/out/helpers/runTests.js'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:902:15)
    at Function.Module._load (internal/modules/cjs/loader.js:746:27)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:75:12)
    at internal/main/run_main_module.js:17:47 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}

I cannot seem to find any documentation around how to compile the TypeScript stuff into test/out/helpers/*.js

mmaietta · 2023-04-04T23:16:19Z

Good to note that the docs need to be updated.
You'll need to compile before running the tests

pnpm compile
TEST_FILES=debTest UPDATE_SNAPSHOT=true pnpm test-linux

…ithout errors about dmg-license cannot be installed. I get this error without this change: ``` npm WARN deprecated puppeteer@2.1.1: < 19.4.0 is no longer supported npm WARN deprecated @braintree/sanitize-url@3.1.0: Potential XSS vulnerability patched in v6.0.0. npm WARN deprecated w3c-hr-time@1.0.2: Use your platform's native performance.now() and performance.timeOrigin. npm ERR! code EBADPLATFORM npm ERR! notsup Unsupported platform for dmg-license@1.0.11: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"}) npm ERR! notsup Valid OS: darwin npm ERR! notsup Valid Arch: any npm ERR! notsup Actual OS: linux npm ERR! notsup Actual Arch: x64 npm ERR! A complete log of this run can be found in: npm ERR! /home/user/.npm/_logs/2023-04-04T23_28_28_809Z-debug.log ``` Changing to an optionalDependency allowed me to proceed without errors.

pnpm-lock.yaml

markizano · 2023-04-07T01:17:54Z

Apologies for the sporadic work here and there on this and thanks so much for the incredible responsiveness on this, @mmaietta !!!

.husky/pre-push

.husky/pre-commit

pnpm-lock.yaml

mmaietta · 2023-04-07T15:29:48Z

packages/app-builder-lib/templates/linux/after-install.tpl

@@ -1,7 +1,11 @@
 #!/bin/bash

 # Link to the binary
-ln -sf '/opt/${sanitizedProductName}/${executable}' '/usr/bin/${executable}'
+if type update-alternatives 2>/dev/null >&1; then
+   update-alternatives --install '/usr/bin/${executable}' '${executable}' '/opt/${sanitizedProductName}/${executable}' 100


I just read this in your changeset. (My apologies for pinging back n forth on this)

If you are having issues on install or get errors about /usr/bin/${executable} already
exists, then simply remove the link and update-alternatives will take over from there.

Do we need to update this code as follows? We can't expect all linux users to know they need to remove the link on their own during the install process

previousLink = "/usr/bin/${executable}" # remove previously existing symlink if applicable [ -L "${previousLink}" ] && [ -e "${previousLink}" ] && rm -f "${previousLink}" update-alternatives --install "${previousLink}" "${executable}" "/opt/${sanitizedProductName}/${executable}" 100

Note: I didn't test this logic, just pulled resources from online and changed ' to ". So you'd need to test this change

What are your thoughts?

No worries, it was an upstream change I wanted to propose into this project and grateful for the thoughtful discussion!

Do we need to update this code as follows? We can't expect all linux users to know they need to remove the link on their own during the install process

I think I see what you mean in the updates here, so I updated the changeset accordingly. This is very valid and for a project as core as this to so many others, it would cause a lot of heartburn.

In this corrected way, the end-user nor middleware have to take any action in consuming this update.

With the above proposed change, it might produce output durring the install about having to correct a broken link as such:

update-alternatives: warning: forcing reinstallation of alternative /opt/${sanitizedProductName}/${executable} because link group ${executable} is broken

on each update.

Single quotes are considered string literals. Double-quotes are considered interpolatable strings. Since the templates are passing through a template processor and not the bash interpreter itself, single quotes in this case would be fine since it's not in-script variables that are being used. Since those variables are "userland" variables outside of the script (interpolated by the template filter), single quotes would also be more secure in this case, so I think it might have been correct. In-script variable use might require double-quotes.
(sorry if this sounds wordy -- I am an instructor on my TikTok & YouTube channels :D )

…umption into descending projects.

mmaietta · 2023-04-07T16:00:38Z

packages/app-builder-lib/templates/linux/after-install.tpl

+  rm -f "/usr/bin/${executable}"
+fi
+
+update-alternatives --install '/usr/bin/${executable}' '${executable}' '/opt/${sanitizedProductName}/${executable}' 100


We still need this check, right?

if type update-alternatives 2>/dev/null >&1; then update-alternatives --install '/usr/bin/${executable}' '${executable}' '/opt/${sanitizedProductName}/${executable}' 100 else ln -sf '/opt/${sanitizedProductName}/${executable}' '/usr/bin/${executable}' fi

Or is that covered by != "/etc/alternatives/${executable}"?

… in the system.

mmaietta · 2023-04-07T16:35:57Z

packages/app-builder-lib/templates/linux/after-install.tpl

-ln -sf '/opt/${sanitizedProductName}/${executable}' '/usr/bin/${executable}'
+if type update-alternatives 2>/dev/null >&1; then
+    # Remove previous link if it doesn't use update-alternatives
+    if [ -L '/usr/bin/${executable}' -a -e '/usr/bin/${executable}' -a "`readlink '/usr/bin/${executable}'`" != '/etc/alternatives/${executable}' ]; then


I don't know what this line means, but I trust 😅

No worries :)

https://ss64.com/bash/test.html

-L - file is symlink.
EXPR -a EXPR - AND operation between expressions.
-e - check if file exists.

Should result in something like (pseudo code ahead):

if ( isSymLink(file) && isExist(file) && readlink(file) != "/etc/alternatives/$exec" )

mmaietta

Kicking off CI

markizano · 2023-04-07T17:03:20Z

@mmaietta -- Thanks so much for working with me to get this into the project!!

github-actions bot added the app-builder-lib label Mar 25, 2023

markizano added 2 commits March 25, 2023 15:49

Adding changeset.

663a57d

husky: This came as a side effect of trying to commit.

bb6dbe8

markizano changed the title ~~Issue #7500: Use update-alternatives instead of symlinks to executable if available.~~ Fix Issue #7500: Use update-alternatives instead of symlinks to executable if available. Mar 25, 2023

markizano changed the title ~~Fix Issue #7500: Use update-alternatives instead of symlinks to executable if available.~~ Fix: Use update-alternatives instead of symlinks for #7500 Mar 25, 2023

markizano changed the title ~~Fix: Use update-alternatives instead of symlinks for #7500~~ fix: Use update-alternatives instead of symlinks for #7500 Mar 25, 2023

markizano mentioned this pull request Mar 25, 2023

Use update-alternatives in Linux to avoid direct symlinks #7500

Closed

markizano added 3 commits April 4, 2023 18:48

Merge remote-tracking branch 'upstream/master' into update-alternatives

33fac7b

Adding the results of the test since they changed these files.

67b0dd4

mmaietta reviewed Apr 5, 2023

View reviewed changes

pnpm-lock.yaml Outdated Show resolved Hide resolved

mmaietta requested changes Apr 7, 2023

View reviewed changes

.husky/pre-push Outdated Show resolved Hide resolved

.husky/pre-commit Outdated Show resolved Hide resolved

pnpm-lock.yaml Outdated Show resolved Hide resolved

Revert change from bce9606

1a45b6c

markizano force-pushed the update-alternatives branch from 7dfc2ef to 1a45b6c Compare April 7, 2023 14:44

markizano requested a review from mmaietta April 7, 2023 14:53

mmaietta reviewed Apr 7, 2023

View reviewed changes

Updated changeset and proposed change to avoid errors and easier cons…

6eff1a0

…umption into descending projects.

mmaietta reviewed Apr 7, 2023

View reviewed changes

markizano added 2 commits April 7, 2023 11:14

We need to continue to consider if update-alternatives is not present…

4097817

… in the system.

Update snapshot after running test suite.

ea5c970

mmaietta reviewed Apr 7, 2023

View reviewed changes

mmaietta approved these changes Apr 7, 2023

View reviewed changes

mmaietta merged commit e83dc81 into electron-userland:master Apr 7, 2023

github-actions bot mentioned this pull request Apr 7, 2023

chore(deploy): Release #7530

Merged

ravicious mentioned this pull request Jun 1, 2023

The same after-install & after-remove scripts are used for deb and RPM packages despite different semantics #7326

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Use `update-alternatives` instead of symlinks for #7500 #7501

fix: Use `update-alternatives` instead of symlinks for #7500 #7501

markizano commented Mar 25, 2023

changeset-bot bot commented Mar 25, 2023 •

edited

Loading

netlify bot commented Mar 25, 2023 •

edited

Loading

markizano commented Mar 25, 2023

mmaietta commented Mar 26, 2023

markizano commented Apr 4, 2023 •

edited

Loading

mmaietta commented Apr 4, 2023 •

edited

Loading

markizano commented Apr 7, 2023

mmaietta Apr 7, 2023

markizano Apr 7, 2023

mmaietta Apr 7, 2023 •

edited

Loading

mmaietta Apr 7, 2023

markizano Apr 7, 2023 •

edited

Loading

mmaietta left a comment

markizano commented Apr 7, 2023

fix: Use update-alternatives instead of symlinks for #7500 #7501

fix: Use update-alternatives instead of symlinks for #7500 #7501

Conversation

markizano commented Mar 25, 2023

changeset-bot bot commented Mar 25, 2023 • edited Loading

🦋 Changeset detected

netlify bot commented Mar 25, 2023 • edited Loading

✅ Deploy Preview for car-park-attendant-cleat-11576 ready!

markizano commented Mar 25, 2023

mmaietta commented Mar 26, 2023

markizano commented Apr 4, 2023 • edited Loading

mmaietta commented Apr 4, 2023 • edited Loading

markizano commented Apr 7, 2023

mmaietta Apr 7, 2023

Choose a reason for hiding this comment

markizano Apr 7, 2023

Choose a reason for hiding this comment

mmaietta Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

mmaietta Apr 7, 2023

Choose a reason for hiding this comment

markizano Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

mmaietta left a comment

Choose a reason for hiding this comment

markizano commented Apr 7, 2023

fix: Use `update-alternatives` instead of symlinks for #7500 #7501

fix: Use `update-alternatives` instead of symlinks for #7500 #7501

changeset-bot bot commented Mar 25, 2023 •

edited

Loading

netlify bot commented Mar 25, 2023 •

edited

Loading

markizano commented Apr 4, 2023 •

edited

Loading

mmaietta commented Apr 4, 2023 •

edited

Loading

mmaietta Apr 7, 2023 •

edited

Loading

markizano Apr 7, 2023 •

edited

Loading