Support up to Electron 8

[glenn2223]

Bring support for Electron 5-8

Needs contributors to build to full support, currently only Windows x32 & x64, OSX x64 and Linux x64

[nschonni]

@glenn2223 thanks for kicking this off!
Few comments:

• Can you try adding some CI for this? I'd suggest taking a look at chore: Add GitHub Actions for Alpine CI #2823 as an example
• I think just updating the case statements with the module should be fine

[nschonni]

Looks like a good start! There are a few things I'd probably suggest changing, but if you start by adding the file to the PR, it will be easier to comment directly on the file.

Here are a few other suggestions:

• Create separate YML files per environment since it seems like there might be some different handling. EX: electron-osx.yml, electron-win.yml, electron-linux.yml
• The binding.node files should be removed from the PR
• Not sure what the best place is for the instructions, maybe just the root of the repo for now, as the .github is for special GitHub related files
• The on should probably be [push, pull_request] instead of create
• Add the actions/checkout@v2 as the first step
• Use the electron version as part of your matrix as an array. The arch looks like it might only need to be in the Windows build. EX:

    strategy:
      matrix:
        electron:
        - 5
        - 6
        - 7
        - 8
        arch:
        - ia32
        - x64
....
    - name: Run on ${{ matrix.os }}
      if: matrix.os != 'macOS-latest'
        run:|
          ./node_modules/.bin/electron-rebuild -v=${{ matrix.electron }} -w node-sass --arch=${{ matrix.arch}}
```
- You can comment out the individual electron version lines to shrink the amount of builds kicked off while you're iterating
- You can take a look at the Alpine PR for an idea of how caching/uploading the binaries might work, but I'd leave that till the end

Hope this helps you iterate one this

[saper]

Don't think it's a problem.

Sure, a script or two may ran fine, but believe me, ABI compatibility problems for C++ components are hard to debug.

Microsoft says that 2015, 2017 and 2019 are binary compatible, something they could not guarantee before. But they say one should use the latest "redistributable" runtime library. Will Electron 5,6,7 bring only 2017 runtime library with itself? If yes, we should make build with 2017 to make sure 2019 is not required.

[glenn2223]

Don't think it's a problem.

Sure, a script or two may ran fine, but believe me, ABI compatibility problems for C++ components are hard to debug.

You're right. When you're right, you're right!

---

Microsoft says that 2015, 2017 and 2019 are binary compatible, something they could not guarantee before. But they say one should use the latest "redistributable" runtime library. Will Electron 5,6,7 bring only 2017 runtime library with itself?(1) If yes, we should make build with 2017 to make sure 2019 is not required.(2)

1. How can I find out?
2. Do we need to move away from GitHub's runners then (ref)?

---

Quick sidebar: When I was running my test I noticed that the install script used node's modules rather than electron's electron. May it be worth checking for process.versions.electron and getting the correct binary on building? Again, only Electron 5+, as they have different modules to node (from what my builds have shown me anyway)

[saper]

I have filed #2868 to work on the general Visual Studio 2019 issue.

[glenn2223]

Could setting npm config set msbuild_path "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Current\Bin\MSBuild.exe" workaround the issue (until we switch to v5)?

Ref lines 153-155 from v3.8.0 configure script & my many failed actions

// Copied from v3.8.0's configure script //

  // GYP doesn't (yet) have support for VS2017, so we force it to VS2015
  // to avoid pulling a floating patch that has not landed upstream.
  // Ref: https://chromium-review.googlesource.com/#/c/433540/

As you can see my attempts fail and the comments (copied in above) seem to indicate that it shouldn't be overridden.

This was my last ditch effort, but failed:

setx PATH "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Current\Bin;%PATH%"
set path="C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Current\Bin;%PATH%"

[nschonni]

I'm OK with this being a CI only setup, there may be some edge cases where local fallback fails, but we can consider this experimental support for v4.
If you really want to try to get the specific toolsets for the versions changed, I'd suggest changing the runs-on values between windows-2016 for VS 2017 and window-2019 for the VS 2019 with a CI only install of newer node-gyp. Not sure this is worth it unless you're finding that the version produced by CI isn't working for your local VS Code install.

[glenn2223]

I'm OK with this being a CI only setup, there may be some edge cases where local fallback fails, but we can consider this experimental support for v4.

Me too!!

If you really want to try to get the specific toolsets for the versions changed, I'd suggest changing the runs-on values between windows-2016 for VS 2017 and window-2019 for the VS 2019 with a CI only install of newer node-gyp.

From saper's comment and reading the linked Microsoft docs, it seems that if we target 5, 6 and 7 with 2019 then we're forcing people who build apps with those electron versions to use a 2019 toolset/VS 2019. This is because those electron builds are compiled with VS 2017 and the binaries require the use of the highest level toolset from any component (To dummy it down, not meaning to be condescending or anything. Electron: 2017 build + binary: 2019 build => 2019 toolset required).
That's even if people are still using VS 2017 for their electron 5, 6 or 7 apps?

Do you think it's worth it? *shrug* just in case? Not sure, maybe overkill?

Not sure this is worth it unless you're finding that the version produced by CI isn't working for your local VS Code install.

The binary works locally with VSCode (latest) on Windows x64, but you can download the forked VSCode extension and be my guinea pig if you like 😆?

[xzyfer]

FWIW I think it makes sense to target this patch for v5. Then only need to support Node 10, 12, 13

[glenn2223]

FWIW I think it makes sense to target this patch for v5. Then only need to support Node 10, 12, 13

Don't suppose there's an eta for v5?

[xzyfer]

If there's no objects from the team I'm keen to tighten the bolts and ship it asap. Thoughts? (Sorry can't @ on my phone)

[saper]

I am against pushing CI-only setup. npm rebuild will no longer work. People will start coming at us when upgrading electron versions. Once we start shipping binaries and electron will become a supported engine I do not see a way for the users to rebuild their bindings themselves if needed.

[glenn2223]

Not related to above

Just as a personal learning experience I created a new branch to see how it would work trying to manipulate the build rather than have a CI. Running an electron prestart script to build the new binding is getting this error. Running the build script in my console works though. Any clues?

Building: C:\Users\Glenn\Documents\GitHub\Sample-Electron\node_modules\electron\dist\electron.exe C:\Users\Glenn\Documents\GitHub\Sample-Electron\node_modules\node-gyp\bin\node-gyp.js rebuild --verbose --target=5.0.13 --dist-url=https://electronjs.org/headers
Build failed with error code: 1

Function call

// if binding not found =>call
child_process.spawnSync(process.execPath, [Path.resolve("./node_modules/node-sass/scripts/build.js"), `--directory=${Path.resolve("./node_modules/node-sass/")}`], {stdio: 'inherit'});

[glenn2223]

If there's no objects from the team I'm keen to tighten the bolts and ship it asap. Thoughts? (Sorry can't @ on my phone)

@xzyfer not that I'm part of the team, but if there's anything I can do to help nail down v5 then let me know. Do you have a milestone with required tasks for v5?

[xzyfer]

@nschonni giving the app a whirl 🤞

@glenn2223 I think we've captured all the breaking changes we're expecting for v5. The last big outstanding change in N-API support but I'd prefer to push that to V6.

IMHO I'm happy to cut the release in the next couple days and roll forward with feature updates like this.

[glenn2223]

Opened v5 PR, closing

#2872