fix: Include swiftshader directory when creating installer for Electron 10+ #375

niik · 2021-01-20T14:41:58Z

This is a continuation of #367 with the intention of including the swiftshader directory (present in Electron 10+) as well as the vk_swiftshader_icd.json file. These paths are required in order for electron to be able to run on systems where GPU acceleration is unavailable.

See #367 (comment):

👋 We (the GitHub Desktop team) is looking into upgrading to Electron 11 and came upon this issue which made us a tad bit nervous as it seems like it would affect Electon 11 as well (right?).

We'd be happy to help in any way we can to get this over the finish line but it's not immediately obvious how.

I haven't looked in detail, but I suspect that this change will fail with older versions of Electron that do not contain the swiftshader files. This module doesn't currently specify the versions of Electron that it supports, so it effectively supports pretty much all versions. If we restrict that, this will require a major version bump. Otherwise the change will need to be fixed so that it works for versions without these files.

@malept Do you have a sense of what your preferred approach would be?

In case the approach would be to special case versions prior to Electron 10 do you envision having two different nuspecttemplate files and conditionally including one or the other?

windows-installer/src/index.ts

Line 107 in 7f6ea86

let templateData = await fs.readFile(path.join(__dirname, '..', 'template.nuspectemplate'), 'utf8');

Or would you rather attempt to add the entries to the existing template at runtime?

And last question (for now), what method would you prefer for detecting whether these files need to be included? Should we just check whether they exist in the appDirectory or would you like to specifically check the bundled electron version?

@malept Is the approach taken here in line with what you'd expect?

Closes #367.

…tshader files

niik · 2021-01-20T15:51:44Z

I believe #376 will allow the test-mac build to pass as well.

malept

Thanks for working on this.

Can the fixtures be zero-sized like the others?

malept · 2021-01-20T16:16:26Z

spec/installer-spec.ts

+
+  log('Verifying contents of .nupkg');
+  const sevenZipPath = path.join(__dirname, '..', 'vendor', '7z.exe');
+  let cmd = sevenZipPath;
+  const args = ['l', nupkgPath];
+
+  if (process.platform !== 'win32') {
+    args.unshift(cmd);
+    const wineExe = process.arch === 'x64' ? 'wine64' : 'wine';
+    cmd = wineExe;
+  }
+
+  const packageContents = await spawn(cmd, args);
+  t.true(packageContents.includes('lib\\net45\\vk_swiftshader_icd.json'));
+  t.true(packageContents.includes('lib\\net45\\swiftshader\\libEGL.dll'));


It would be nice if there was a test that didn't add the swiftshader files as well, to prevent regressions.

Agreed, I'll take a stab at it!

I encountered an issue where the two tests caused a race condition trying to delete the Squirrel.exe file in the beforeEach so I tweaked the approach to always copying the fixture app directory to a temporary location before packaging.

niik · 2021-01-20T17:04:03Z

Can the fixtures be zero-sized like the others?

I just copied one of other binary fixtures here which I guess wasn't zero sized. Looking at the fixtures directory there seems to be a mix of empty files and binary-like files. I'll empty them tho!

…xist

…lude-swiftshader

malept

This looks fine now, I just have some suggestions about temporary directory naming.

spec/installer-spec.ts

malept · 2021-01-20T17:56:52Z

spec/installer-spec.ts

+  return process.platform !== 'win32'
+    ? spawn(process.arch === 'x64' ? 'wine64' : 'wine', [sevenZipPath, ...args])
+    : spawn(sevenZipPath, args);


Normally I'd ask to refactor this to look a bit more like how it's done elsewhere in the module (I do not agree with multi-line ternary statements), but I have a PR to drastically refactor how spawn works in #373 so it's kind of a moot point.

Sorry, I tried to stay true to the current coding style but old habits die hard, I'll extract the wine binary name into a variable.

spec/installer-spec.ts

Co-authored-by: Mark Lee <[email protected]>

sergiou87 · 2021-02-01T16:08:51Z

Hi @malept!

Did you have a chance to take a look at @niik's changes? This PR is blocking desktop/desktop#11142, so please let me know if there is anything I can do to help getting this merged! 🙏

Thank you! ❤️

malept

Thanks for your patience, this will go out in a new release shortly.

electron-bot · 2021-02-10T17:30:59Z

🎉 This PR is included in version 4.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

niik added 4 commits January 20, 2021 14:57

Add swiftshader files to nuspec template when available

cc715be

Fix lint warnings

255d8d1

Shuffle things around in the fixture app to mimic Electron 10 layout

2db4aab

Try verifying that the final nuget package actually includes the swif…

dd035a2

…tshader files

niik changed the title ~~Conditionally include swiftshader directory~~ fix: Include swiftshader directory when creating installer for Electron 10+ Jan 20, 2021

niik added 3 commits January 20, 2021 15:54

No clue if this is gonna work

772635f

Lint

f517443

Someone take away my coding privileges pls

0501a27

malept requested changes Jan 20, 2021

View reviewed changes

niik added 3 commits January 20, 2021 18:40

Verify that installer is created when swiftshader directory doesn't e…

3368341

…xist

Merge remote-tracking branch 'upstream/master' into conditionally-inc…

ffe8e31

…lude-swiftshader

Empty fixture files

a0b2b70

niik requested a review from malept January 20, 2021 17:47

malept reviewed Jan 21, 2021

View reviewed changes

niik and others added 2 commits January 22, 2021 10:40

🔥 double ternaries

b5c6cc3

Apply suggestions from code review

b36f263

Co-authored-by: Mark Lee <[email protected]>

niik requested a review from malept January 22, 2021 09:43

niik mentioned this pull request Jan 22, 2021

Upgrade to Electron 11 desktop/desktop#11142

Merged

5 tasks

malept approved these changes Feb 10, 2021

View reviewed changes

malept merged commit 7aacb15 into electron:master Feb 10, 2021

electron-bot added the released label Feb 10, 2021

sergiou87 mentioned this pull request Feb 11, 2021

Bump electron-winstaller to 4.0.2 desktop/desktop#11563

Merged

dependabot bot mentioned this pull request Mar 16, 2021

build(deps): bump electron-winstaller from 4.0.1 to 5.0.0 dopry/electron-forge#6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Include swiftshader directory when creating installer for Electron 10+ #375

fix: Include swiftshader directory when creating installer for Electron 10+ #375

niik commented Jan 20, 2021 •

edited by malept

Loading

niik commented Jan 20, 2021 •

edited

Loading

malept left a comment

malept Jan 20, 2021

niik Jan 20, 2021

niik Jan 20, 2021

niik commented Jan 20, 2021

malept left a comment

malept Jan 20, 2021

niik Jan 22, 2021

sergiou87 commented Feb 1, 2021

malept left a comment

electron-bot commented Feb 10, 2021

fix: Include swiftshader directory when creating installer for Electron 10+ #375

fix: Include swiftshader directory when creating installer for Electron 10+ #375

Conversation

niik commented Jan 20, 2021 • edited by malept Loading

niik commented Jan 20, 2021 • edited Loading

malept left a comment

Choose a reason for hiding this comment

malept Jan 20, 2021

Choose a reason for hiding this comment

niik Jan 20, 2021

Choose a reason for hiding this comment

niik Jan 20, 2021

Choose a reason for hiding this comment

niik commented Jan 20, 2021

malept left a comment

Choose a reason for hiding this comment

malept Jan 20, 2021

Choose a reason for hiding this comment

niik Jan 22, 2021

Choose a reason for hiding this comment

sergiou87 commented Feb 1, 2021

malept left a comment

Choose a reason for hiding this comment

electron-bot commented Feb 10, 2021

niik commented Jan 20, 2021 •

edited by malept

Loading

niik commented Jan 20, 2021 •

edited

Loading