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.

Sign up for GitHub

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

Jump to bottom

Support up to Electron 8 #2862

Closed
wants to merge 47 commits into from
Closed

Support up to Electron 8 #2862

Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
f57f4b4
Support upto Electron 8 (Windows ONLY)
glenn2223 Mar 16, 2020
9c5aeb6
Docs to expand on Electron Support
glenn2223 Mar 16, 2020
7f5b6bf
Update Electron-Support.md
glenn2223 Mar 16, 2020
90e697d
Update Electron-Support.md
glenn2223 Mar 16, 2020
e6b1a43
Reverted isSupportedEnvironment
glenn2223 Mar 16, 2020
0661d3b
Create electron-win.yml
glenn2223 Mar 17, 2020
92b7ef0
Delete Electron-Support.md
glenn2223 Mar 17, 2020
1760a4c
Removed bindings
glenn2223 Mar 17, 2020
ed1c343
Merge branch 'master' of https://github.com/glenn2223/node-sass
glenn2223 Mar 17, 2020
97a903b
Update electron-win.yml
glenn2223 Mar 17, 2020
2bc2261
Updated node-gyp to 6.1.0
glenn2223 Mar 17, 2020
5fe05fa
Renamed artifacts to almost match
glenn2223 Mar 17, 2020
80a5070
Workflow changes
glenn2223 Mar 17, 2020
7368214
Update electron-linux.yml
glenn2223 Mar 17, 2020
f3aaea6
Workflow updates
glenn2223 Mar 17, 2020
18687aa
Workflow updates
glenn2223 Mar 17, 2020
499e066
Workflow updates
glenn2223 Mar 17, 2020
9642e7b
Fixes
glenn2223 Mar 17, 2020
d95fc8a
Update electron-linux.yml
glenn2223 Mar 17, 2020
9d01660
Update electron-linux.yml
glenn2223 Mar 17, 2020
69ddf4c
Update electron-linux.yml
glenn2223 Mar 17, 2020
2cc27f0
Update electron-linux.yml
glenn2223 Mar 17, 2020
480890d
Update electron-linux.yml
glenn2223 Mar 17, 2020
72ed801
Update electron-linux.yml
glenn2223 Mar 17, 2020
138d277
Dropped x32 from linux workflow
glenn2223 Mar 17, 2020
165f68a
Reverted to electron rebuild
glenn2223 Mar 17, 2020
2eb370e
Merge branch 'master' of https://github.com/glenn2223/node-sass
glenn2223 Mar 17, 2020
ed333ad
Update electron-win.yml
glenn2223 Mar 17, 2020
4da3d46
Update workflows
glenn2223 Mar 17, 2020
17cfbc0
Workflow updates
glenn2223 Mar 17, 2020
a1db6da
Workflow updates
glenn2223 Mar 17, 2020
818b048
Removed unrequired dependancy
glenn2223 Mar 17, 2020
9faff00
Update electron-win.yml
glenn2223 Mar 17, 2020
23927c8
Linux test
glenn2223 Mar 17, 2020
7c35440
Update electron-linux.yml
glenn2223 Mar 17, 2020
f1bdcea
Final workflow update
glenn2223 Mar 17, 2020
78048c9
Windows workflow correction
glenn2223 Mar 18, 2020
2e93037
Update electron-win.yml
glenn2223 Mar 18, 2020
c65aa3c
Small tweaks to workers
glenn2223 Mar 18, 2020
403a152
Workflow update
glenn2223 Mar 19, 2020
a7543a1
Windows workflow update
glenn2223 Mar 19, 2020
96f12b6
Update electron-win.yml
glenn2223 Mar 19, 2020
3e7526a
Update electron-win.yml
glenn2223 Mar 19, 2020
45801f7
Final windows workflow change
glenn2223 Mar 19, 2020
99a6b2f
Final windows workflow change
glenn2223 Mar 19, 2020
0ec38a7
Test 8
glenn2223 Mar 20, 2020
d8bda5a
Reverted windows workflow
glenn2223 Mar 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 47 additions & 0 deletions .github/workflows/electron-linux.yml
View file
Open in desktop
@@ -0,0 +1,47 @@
name: Electron Rebuild CI - Linux (x32 & x64)

on: [push, pull_request]

jobs:
build:

runs-on: ubuntu-latest

strategy:
matrix:
electron:
- 5
- 6
- 7
- 8
include:
# includes a new variable for each electron (essentially a switch statement)
- electron: 5
nodeVersion: 70
- electron: 6
nodeVersion: 73
- electron: 7
nodeVersion: 75
- electron: 8
nodeVersion: 76

steps:
- uses: actions/checkout@v2
- name: Use Node.js
uses: actions/setup-node@v1
with:
os: ${{ matrix.os }}
- name: Install packages
run: npm install --unsafe-perm
- name: Install electron-rebuild
run: npm install --save-dev electron-rebuild@latest
- name: Run on ${{ matrix.os }} - Electron v${{ matrix.electron }} (Node ${{ matrix.nodeVersion }})
run: ./node_modules/.bin/electron-rebuild -v=${{ matrix.electron }} -w node-sass
glenn2223 marked this conversation as resolved.
Show resolved Hide resolved
- uses: actions/upload-artifact@v1
with:
name: linux_x64_${{ matrix.nodeVersion }}_binding.node
path: build/Release/binding.node
# npm test failing on pre-existing master issues, commenting out
Copy link
Contributor

Choose a reason for hiding this comment

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

What the pre-existing failures?

Copy link
Author

@glenn2223 glenn2223 Mar 18, 2020 •
edited

Choose a reason for hiding this comment

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

If you see this run
https://github.com/glenn2223/node-sass/runs/514109989?check_suite_focus=true#step:9:11601

    cli node-sass sass/ --output css/ should not error if output directory is a symlink:

      Uncaught AssertionError [ERR_ASSERTION]: [] deepEqual [ 'nested', 'one.css', 'two.css' ]
      + expected - actual

      -[]
      +[
      +  "nested"
      +  "one.css"
      +  "two.css"
      +]

I'll add the data from a local run tomorrow (in GMT here) as I think the error was different on that

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, the other issue I could think that it's running into is that the npm test is still using the regular system NodeJS version to execute, but the binding is setup for an electron environment. Not familiar with how to properly test Electron, but is there something like export NODE=electron_binary, similar to what python tests do with export PYTHON=python3 for making sure a particular version is used.

If it really is a CLI issue with Electron, but Electron won't use the test, maybe there is a way to just skip the test on Electron, but run the rest.

Copy link
Author

Choose a reason for hiding this comment

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

I'll add the data from a local run tomorrow (in GMT here) as I think the error was different on that

  6084 passing (31s)
  753 pending
  2 failing

  1) cli node-sass sass/ --output css/ should not error if output directory is a symlink:
     Error: EEXIST: file already exists, mkdir 'C:\Users\Glenn\Documents\GitHub\node-sass\test\fixtures\input-directory\css'
      at Object.mkdirSync (fs.js:860:3)
      at Context.<anonymous> (test\cli.js:582:10)
      at processImmediate (internal/timers.js:456:21)

  2) cli node-sass --follow --output output-dir input-dir should compile with the --follow option:
     Error: EEXIST: file already exists, mkdir 'C:\Users\Glenn\Documents\GitHub\node-sass\test\fixtures\follow\input-dir'
      at Object.mkdirSync (fs.js:860:3)
      at Context.<anonymous> (test\cli.js:623:10)
      at processImmediate (internal/timers.js:456:21)



npm ERR! Test failed.  See above for more details.

The above is the result when I run npm test on the original repository (no changes)
Have it open in VSCode Winx62, using vendor node from win32-x64-79 (I have Node 13.10.1)

Copy link
Author

Choose a reason for hiding this comment

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

OK, the other issue I could think that it's running into is that the npm test is still using the regular system NodeJS version to execute, but the binding is setup for an electron environment. Not familiar with how to properly test Electron, but is there something like export NODE=electron_binary, similar to what python tests do with export PYTHON=python3 for making sure a particular version is used.

If it really is a CLI issue with Electron, but Electron won't use the test, maybe there is a way to just skip the test on Electron, but run the rest.

TBH, I don't know why I'm running npm test as we can't test it in that environment (unless I missed something)

The test is running on the system Node but still using the binary it downloaded from GitHub to test. This is because running electron-rebuild places the electron binary in the bin & build/Release folders, completely separate from where the current binary is (obv).

I tried (even though I knew the result) to replace the binary in the vendor folder and see what would happen; you guessed it, I got the below error: (essentially trying to run Electron 8 on Node 13)

Binary has a problem: Error: The module '\\?\C:\Users\Glenn\Documents\GitHub\node-sass\vendor\win32-x64-79\binding.node'
was compiled against a different Node.js version using
NODE_MODULE_VERSION 76. This version of Node.js requires
NODE_MODULE_VERSION 79. Please try re-compiling or re-installing
the module (for instance, using `npm rebuild` or `npm install`).

I think we should drop the npm test as the binary it is rebuilding off must be functional otherwise it wouldn't be accessible in a public release (or at least that's what we all hope 😅)

Do you agree?

#- run: npm test
# env:
# CI: true
47 changes: 47 additions & 0 deletions .github/workflows/electron-osx.yml
View file
Open in desktop
@@ -0,0 +1,47 @@
name: Electron Rebuild CI - OSX (x64)

on: [push, pull_request]

jobs:
build:

runs-on: macos-latest

strategy:
matrix:
electron:
- 5
- 6
- 7
- 8
include:
# includes a new variable for each electron (essentially a switch statement)
- electron: 5
nodeVersion: 70
- electron: 6
nodeVersion: 73
- electron: 7
nodeVersion: 75
- electron: 8
nodeVersion: 76

steps:
- uses: actions/checkout@v2
- name: Use Node.js
uses: actions/setup-node@v1
with:
os: ${{ matrix.os }}
- name: Install packages
run: npm install --unsafe-perm
- name: Install electron-rebuild
run: npm install --save-dev electron-rebuild@latest
- name: Run on ${{ matrix.os }} - Electron v${{ matrix.electron }} (Node ${{ matrix.nodeVersion }})
run: ./node_modules/.bin/electron-rebuild -v=${{ matrix.electron }} -w node-sass
glenn2223 marked this conversation as resolved.
Show resolved Hide resolved
- uses: actions/upload-artifact@v1
with:
name: darwin_x64_${{ matrix.nodeVersion }}_binding.node
path: build/release/binding.node
# npm test failing on pre-existing master issues, commenting out
#- run: npm test
# env:
# CI: true
54 changes: 54 additions & 0 deletions .github/workflows/electron-win.yml
View file
Open in desktop
@@ -0,0 +1,54 @@
name: Electron Rebuild CI - Windows (x32 & x64)

on: [push, pull_request]

jobs:
build:

runs-on: windows-latest

strategy:
matrix:
electron:
- 5
- 6
- 7
- 8
include:
# includes a new variable for each electron (essentially a switch statement)
- electron: 5
nodeVersion: 70
- electron: 6
nodeVersion: 73
- electron: 7
nodeVersion: 75
- electron: 8
nodeVersion: 76
arch:
- ia32
- x64

steps:
- uses: actions/checkout@v2
- name: Use Node.js
uses: actions/setup-node@v1
with:
os: ${{ matrix.os }}
- name: Install packages
run: npm install --unsafe-perm
- name: Install electron-rebuild
run: npm install --save-dev electron-rebuild@latest
- name: Run on ${{ matrix.os }} - Electron v${{ matrix.electron }} (Node ${{ matrix.nodeVersion }})
run: ./node_modules/.bin/electron-rebuild -v=${{ matrix.electron }} -w node-sass
- uses: actions/upload-artifact@v1
with:
name: win32_${{ matrix.arch }}_${{ matrix.nodeVersion }}_binding.node
path: build/release/binding.node
- uses: actions/upload-artifact@v1
with:
name: win32_${{ matrix.arch }}_${{ matrix.nodeVersion }}_binding.pdb
path: build/release/binding.pdb
# npm test failing on pre-existing master issues, commenting out
glenn2223 marked this conversation as resolved.
Show resolved Hide resolved
#- run: npm test
# env:
# CI: true
4 changes: 4 additions & 0 deletions lib/extensions.js
View file
Open in desktop
Expand Up @@ -77,7 +77,11 @@ function getHumanNodeVersion(abi) {
case 59: return 'Node.js 9.x';
case 64: return 'Node.js 10.x';
case 67: return 'Node.js 11.x';
case 70: return 'Electron 5.x';
case 72: return 'Node.js 12.x';
case 73: return 'Electron 6.x';
case 75: return 'Electron 7.x';
case 76: return 'Electron 8.x';
case 79: return 'Node.js 13.x';
default: return false;
}
Expand Down