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

[WIP] Make sure libsass headers are read first #2070

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

saper
Copy link
Member

@saper saper commented Aug 17, 2017

In the case libsass is installed system-wide
(e.g. the headers are in /usr/local/include) and
node requires reading headers from the same path
for the unbundled shared libraries like libuv,
make sure -I/usr/local/include will not take
precedence over our local bundles libsass headers.

In the case libsass is installed system-wide
(e.g. the headers are in /usr/local/include) *and*
node requires reading headers from the same path
for the unbundled shared libraries like libuv,
make sure -I/usr/local/include will not take
precedence over our local bundles libsass headers.
@@ -35,8 +35,9 @@
'GCC_ENABLE_CPP_EXCEPTIONS': 'NO',
'MACOSX_DEPLOYMENT_TARGET': '10.7'
},
'include_dirs': [
'include_dirs+': [
Copy link
Contributor

Choose a reason for hiding this comment

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

Could/should this be done below in the libsass_ext condition? Not too familar with the gyp syntax though

@saper
Copy link
Member Author

saper commented Aug 18, 2017

Yes. That is how it should be, but it does not work. Paths added from behind the condition will be merged in, but they get appended regardless of +

We need this because I want to propose node-gyp change to use potentially systemwide paths for libuv and such. If an older libsass is installed globally old header files will be used instead of our new ones.

@nschonni
Copy link
Contributor

@saper I just flipped this to Draft since it had "WIP" in the title. If it's good to go, feel free to flip it back

jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this pull request Apr 7, 2024
Trailing commas in parameters and arguments was introduced in 3.5.

Fixes sass#2070
Spec sass/sass-spec#1090
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.

2 participants