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

Conversation

saper
Copy link
Member

@saper saper commented Apr 21, 2021

No description provided.

- Bump to 5.1.0
- Add CI builds for 16
- Add extension lookup for module 93
@saper saper force-pushed the upgrade-cpp-standard-to-14 branch from 9be2da4 to 12d935a Compare April 21, 2021 13:20
@saper saper marked this pull request as draft April 21, 2021 13:20
@@ -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

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.

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.

dieunity pushed a commit to dieunity/spearmint_LA42 that referenced this pull request Apr 24, 2021
…al node-sass upgrade is being discussed by contributors (changing C++ binding configuration in node_modules sass/node-sass#3091), but due to project timeline, removing node-sass entirely is the solution chosen

Co-authored-by: Justin Baik <bij3377@gmail.com>
Co-authored-by: Max Weisenberger <germanbluemax@gmail.com>
Co-authored-by: Mo Hmaidi <mhmaidi789@gmail.com>
@saper saper mentioned this pull request Apr 25, 2021
4 tasks
@dcunited001
Copy link

dcunited001 commented Apr 30, 2021

if i simply pass CLANG_CXX_LANGUAGE_STANDARD=c++14 to the build process is this sufficient? how do i forward the params to make/etc to do that?

i'm running on archlinux. i'll probably just downgrade node to v15 until this is settled.

@polarbirke
Copy link

Is there still movement on this? The need for Node v16 compatibility is becoming more important now that it is the active LTS; Github Actions have updated a few days ago (actions/runner-images#4446), for example.

@polarbirke
Copy link

Is there still movement on this?

Never mind, https://github.com/sass/node-sass/releases/tag/v6.0.0 seems to have solved this.

@nschonni nschonni closed this Dec 9, 2021
Friendly-users referenced this pull request in Friendly-users/node-sass Jul 9, 2024
-----
It is inappropriate to include political and offensive content in public code repositories.

Public code repositories should be neutral spaces for collaboration and community, free from personal or political views that could alienate or discriminate against others. Political content, especially that which targets or disparages minority groups, can be harmful and divisive. It can make people feel unwelcome and unsafe, and it can create a hostile work environment.

Please refrain from adding such content to public code repositories.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants