Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

[DNM] Bump C++ standard to C++14 #3091

Closed
wants to merge 7 commits into from
Closed

[DNM] Bump C++ standard to C++14 #3091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a71f3d8
chore: Add Node 16 support
nschonni Apr 20, 2021
6939667
chore: Bump dependabot limit
nschonni Dec 11, 2020
3370c11
build(deps): bump actions/setup-node from v2.1.2 to v2.1.3
dependabot[bot] Dec 11, 2020
9f6799a
build(deps): bump actions/setup-node from v2.1.3 to v2.1.4
dependabot[bot] Dec 21, 2020
ac60d54
Update TROUBLESHOOTING.md
independencyinjection Dec 22, 2020
3b38b97
build(deps): bump actions/setup-node from v2.1.4 to v2.1.5
dependabot[bot] Mar 1, 2021
12d935a
[DNM] Bump C++ standard to C++14
saper Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/dependabot.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ version: 2
updates:
- package-ecosystem: "npm"
directory: "/"
open-pull-requests-limit: 99
schedule:
interval: "weekly"
- package-ecosystem: "github-actions"
directory: "/"
open-pull-requests-limit: 99
schedule:
interval: "weekly"
3 changes: 3 additions & 0 deletions .github/workflows/alpine.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ jobs:
- 12
- 14
- 15
- 16

include:
- node: 10
Expand All @@ -29,6 +30,8 @@ jobs:
alpine: "3.10"
- node: 15
alpine: "3.10"
- node: 16
alpine: "3.10"

steps:
- name: Install Alpine build tools
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/lint-js.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ jobs:
steps:
- uses: actions/checkout@v2

- uses: actions/setup-node@v2.1.2
- uses: actions/setup-node@v2.1.5

- name: Install packages
run: npm install --unsafe-perm
Expand Down
8 changes: 7 additions & 1 deletion .github/workflows/linux.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ jobs:
- 12
- 14
- 15
- 16

include:
- node: 10
Expand All @@ -36,12 +37,17 @@ jobs:
gcc: "gcc-6"
gpp: "g++-6"
os: ubuntu-18.04
- node: 16
gcc: "gcc-8"
gpp: "g++-8"
os: ubuntu-18.04


steps:
- uses: actions/checkout@v2

- name: Setup Node.js environment
uses: actions/setup-node@v2.1.2
uses: actions/setup-node@v2.1.5
with:
node-version: ${{ matrix.node }}

Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/macos.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,13 @@ jobs:
- 12
- 14
- 15
- 16

steps:
- uses: actions/checkout@v2

- name: Setup Node.js environment
uses: actions/setup-node@v2.1.2
uses: actions/setup-node@v2.1.5
with:
node-version: ${{ matrix.node }}

Expand Down
5 changes: 4 additions & 1 deletion .github/workflows/windows.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ jobs:
- 12
- 14
- 15
- 16

include:
- node: 10
Expand All @@ -28,12 +29,14 @@ jobs:
os: windows-2016
- node: 15
os: windows-2019
- node: 16
os: windows-2019

steps:
- uses: actions/checkout@v2

- name: Setup Node.js environment
uses: actions/setup-node@v2.1.2
uses: actions/setup-node@v2.1.5
with:
node-version: ${{ matrix.node }}

Expand Down
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ Below is a quick guide for minimum and maximum support supported version of node

NodeJS | Supported node-sass version | Node Module
--------|-----------------------------|------------
Node 16 | 5.1+ | 93
Node 15 | 5.0+ | 88
Node 14 | 4.14+ | 83
Node 13 | 4.13+, <5.0 | 79
Expand Down
2 changes: 1 addition & 1 deletion TROUBLESHOOTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ should always follow these steps before opening a new issue.

### 404s

If you see a 404 when trying to install node-sass, this indicates that your trying
If you see a 404 when trying to install node-sass, this indicates that you're trying
to install a version of node-sass that doesn't support your version of NodeJS, or
uses an alternate V8 environment (Meteor, Electron, etc...) that isn't supported
by node-sass.
Expand Down
6 changes: 6 additions & 0 deletions appveyor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@
- nodejs_version: 15
GYP_MSVS_VERSION: 2019
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
- nodejs_version: 16
GYP_MSVS_VERSION: 2019
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019

install:
# https://www.appveyor.com/docs/lang/nodejs-iojs/#installing-any-version-of-nodejs-or-iojs
Expand Down Expand Up @@ -124,6 +127,9 @@
- nodejs_version: 15
GYP_MSVS_VERSION: 2019
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
- nodejs_version: 16
GYP_MSVS_VERSION: 2019
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019

install:
# https://www.appveyor.com/docs/lang/nodejs-iojs/#installing-any-version-of-nodejs-or-iojs
Expand Down
4 changes: 2 additions & 2 deletions binding.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
}
},
'xcode_settings': {
'CLANG_CXX_LANGUAGE_STANDARD': 'c++11',
'CLANG_CXX_LANGUAGE_STANDARD': 'c++14',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should/do we need to condition this so it only applies to Node 16?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. They obviously build, but we should check if they work on old platforms.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the other patch that @xzyfer pointed to in the other thread just dropped the line entirely. I wonder if that makes sense either.
I'm good with going with the 14 if it makes sense, since I think you've got more c++ experience than me 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather not change the things for previous versions because we know the resulting binaries are problem free and I have the rocker containers to build them ready

Copy link

Choose a reason for hiding this comment

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

Our team is using node-sass v4.12.0 and have been unable to compile node-sass.

We manually changed line 31 to 'CLANG_CXX_LANGUAGE_STANDARD': 'c++14' and we were able to overcome the 'remove_cv_t' error with xcode.

Copy link
Member Author

Choose a reason for hiding this comment

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

We manually changed line 31 to 'CLANG_CXX_LANGUAGE_STANDARD': 'c++14' and we were able to overcome the 'remove_cv_t' error with xcode.

Which node version were you on?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there is canonical answer for which cxx which should be targeting for each Node version at https://github.com/nodejs/node/blob/master/BUILDING.md#official-binary-platforms-and-toolchains

Lots of the decisions in our gyp configuration were made back in the node 0.11 / 0.12 days and cludged on since then to just make it work from them on.

Copy link

Choose a reason for hiding this comment

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

We manually changed line 31 to 'CLANG_CXX_LANGUAGE_STANDARD': 'c++14' and we were able to overcome the 'remove_cv_t' error with xcode.

Which node version were you on?

We are on Node v14.15.1.

After reading the compatible version table on the readme, we deleted node_modules folder and package-lock.json, upgraded to node-sass 4.14 and still encountered the same error and fixed it with the same solution.

'CLANG_CXX_LIBRARY': 'libc++',
'OTHER_LDFLAGS': [],
'GCC_ENABLE_CPP_EXCEPTIONS': 'NO',
Expand Down Expand Up @@ -81,7 +81,7 @@
}],
['OS!="win"', {
'cflags_cc+': [
'-std=c++0x'
'-std=c++14'
]
}]
]
Expand Down
1 change: 1 addition & 0 deletions lib/extensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ function getHumanNodeVersion(abi) {
case 79: return 'Node.js 13.x';
case 83: return 'Node.js 14.x';
case 88: return 'Node.js 15.x';
case 93: return 'Node.js 16.x';
default: return false;
}
}
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "node-sass",
"version": "5.0.0",
"version": "5.1.0",
"libsass": "3.5.5",
"description": "Wrapper around libsass",
"license": "MIT",
Expand Down
4 changes: 2 additions & 2 deletions src/libsass.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@
'conditions': [
['OS=="mac"', {
'xcode_settings': {
'CLANG_CXX_LANGUAGE_STANDARD': 'c++11',
'CLANG_CXX_LANGUAGE_STANDARD': 'c++14',
'CLANG_CXX_LIBRARY': 'libc++',
'OTHER_LDFLAGS': [],
'GCC_ENABLE_CPP_EXCEPTIONS': 'YES',
Expand All @@ -105,7 +105,7 @@
}],
['OS!="win"', {
'cflags_cc+': [
'-std=c++0x'
'-std=c++14'
]
}]
]
Expand Down