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.

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

[WIP] Use post_config.gypi to use shared libraries #1264

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

saper
Copy link
Contributor

@saper saper commented Aug 17, 2017

Node can be compiled with external shared libraries;
in this case append their compile and linker flags last.

@bnoordhuis
Copy link
Member

When is this a problem and why does it require a new file?

@saper
Copy link
Contributor Author

saper commented Aug 18, 2017

I try to make node-sass to compile in a situation when libuv is unbundled. I need to supply system include directory (in my case -I /usr/local/include) which make cause certain pollution. For example, I've had an older version of https://github.com/sass/libsass installed in /usr/local and after adding -I /usr/local/include compilation failed.

What I am trying to do is to tell sass/node-sass#2070 to put libsass headers first and this patch is needed to make sure system include path goes last. So we get this order:

-I _added by the binary module_ -I _needed by node itself_

I do not see a way to tell gyp "add those directories at the end of the list". When not using "+" (prepend) qualifier it seems to merge directories in the order of include files - so I have added the directories to the post_config.gypi that should be included after addon.gypi.

I am experimenting here to make it work, this PR combined with sass/node-sass#2070 does the job.

There are may be better alternatives, like removing include_dirs altogether from addon.gypi and building the include_dirs list once into the config.gypi. I would like to also avoid eventuality to adjust binary modules' binding.gyp just for this reason. The problem is that the binding.gyp is the main file and I think it is processed at the end; but extension authors shouldn't be asked to always use include_dirs+ in their binding files just to fix this relatively rare corner case.

@saper
Copy link
Contributor Author

saper commented Aug 18, 2017

> node -p process.config
{ target_defaults: 
   { cflags: [],
     default_configuration: 'Release',
     defines: [],
     include_dirs: 
      [ '/usr/local/include',
        '/usr/local/include',
        '/usr/local/include' ],
     libraries: 
      [ '-lz',
        '-L/usr/local/lib',
        '-luv',
        '-L/usr/local/lib',
        '-lcares',
        '-L/usr/local/lib',
        '-licui18n',
        '-licuuc',
        '-licudata' ] },
  variables: 
   { asan: 0,
     coverage: false,
     debug_devtools: 'node',
     force_dynamic_crt: 0,
     host_arch: 'x64',
     icu_gyp_path: 'tools/icu/icu-system.gyp',
     icu_small: false,
     llvm_version: '3.4',
     node_byteorder: 'little',
     node_enable_d8: false,
     node_enable_v8_vtunejit: false,
     node_install_npm: false,
     node_module_version: 57,
     node_no_browser_globals: false,
     node_prefix: '/usr/local',
     node_release_urlbase: '',
     node_shared: false,
     node_shared_cares: true,
     node_shared_http_parser: false,
     node_shared_libuv: true,
     node_shared_openssl: false,
     node_shared_zlib: true,
     node_tag: '',
     node_use_bundled_v8: true,
     node_use_dtrace: false,
     node_use_etw: false,
     node_use_lttng: false,
     node_use_openssl: true,
     node_use_perfctr: false,
     node_use_v8_platform: true,
     node_without_node_options: false,
     openssl_fips: '',
     openssl_no_asm: 0,
     shlib_suffix: 'so.57',
     target_arch: 'x64',
     uv_parent_path: '/deps/uv/',
     uv_use_dtrace: false,
     v8_enable_gdbjit: 0,
     v8_enable_i18n_support: 1,
     v8_enable_inspector: 1,
     v8_no_strict_aliasing: 1,
     v8_optimized_debug: 0,
     v8_promise_internal_field_count: 1,
     v8_random_seed: 0,
     v8_trace_maps: 0,
     v8_use_snapshot: false,
     want_separate_host_toolset: 0,
     want_separate_host_toolset_mkpeephole: 0 } }

Here is what happens (building sass/node-sass@4a0f3d0 with https://github.com/nodejs/node-gyp/releases/tag/v3.6.2 when using node v8.2.1 from FreeBSD ports :

  c++ '-DNODE_GYP_MODULE_NAME=libsass' '-DUSING_UV_SHARED=1' '-DUSING_V8_SHARED=1' '-DV8_DEPRECATION_WARNINGS=1' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-DLIBSASS_VERSION="3.5.0.beta.2"' -I/usr/local/include/node -I/usr/local/src -I/usr/local/deps/uv/include -I/usr/local/deps/v8/include -I../src/libsass/include  -fPIC -pthread -Wall -Wextra -Wno-unused-parameter -m64 -O3 -fno-omit-frame-pointer -std=gnu++0x -std=c++0x -fexceptions -frtti -MMD -MF ./Release/.deps/Release/obj.target/libsass/src/libsass/src/values.o.d.raw   -c -o Release/obj.target/libsass/src/libsass/src/values.o ../src/libsass/src/values.cpp
  rm -f Release/obj.target/src/sass.a && ar crs Release/obj.target/src/sass.a Release/obj.target/libsass/src/libsass/src/ast.o Release/obj.target/libsass/src/libsass/src/ast_fwd_decl.o Release/obj.target/libsass/src/libsass/src/base64vlq.o Release/obj.target/libsass/src/libsass/src/bind.o Release/obj.target/libsass/src/libsass/src/cencode.o Release/obj.target/libsass/src/libsass/src/check_nesting.o Release/obj.target/libsass/src/libsass/src/color_maps.o Release/obj.target/libsass/src/libsass/src/constants.o Release/obj.target/libsass/src/libsass/src/context.o Release/obj.target/libsass/src/libsass/src/cssize.o Release/obj.target/libsass/src/libsass/src/emitter.o Release/obj.target/libsass/src/libsass/src/environment.o Release/obj.target/libsass/src/libsass/src/error_handling.o Release/obj.target/libsass/src/libsass/src/eval.o Release/obj.target/libsass/src/libsass/src/expand.o Release/obj.target/libsass/src/libsass/src/extend.o Release/obj.target/libsass/src/libsass/src/file.o Release/obj.target/libsass/src/libsass/src/functions.o Release/obj.target/libsass/src/libsass/src/inspect.o Release/obj.target/libsass/src/libsass/src/json.o Release/obj.target/libsass/src/libsass/src/lexer.o Release/obj.target/libsass/src/libsass/src/listize.o Release/obj.target/libsass/src/libsass/src/memory/SharedPtr.o Release/obj.target/libsass/src/libsass/src/node.o Release/obj.target/libsass/src/libsass/src/output.o Release/obj.target/libsass/src/libsass/src/parser.o Release/obj.target/libsass/src/libsass/src/plugins.o Release/obj.target/libsass/src/libsass/src/position.o Release/obj.target/libsass/src/libsass/src/prelexer.o Release/obj.target/libsass/src/libsass/src/remove_placeholders.o Release/obj.target/libsass/src/libsass/src/sass.o Release/obj.target/libsass/src/libsass/src/sass2scss.o Release/obj.target/libsass/src/libsass/src/sass_context.o Release/obj.target/libsass/src/libsass/src/sass_functions.o Release/obj.target/libsass/src/libsass/src/sass_util.o Release/obj.target/libsass/src/libsass/src/sass_values.o Release/obj.target/libsass/src/libsass/src/source_map.o Release/obj.target/libsass/src/libsass/src/subset_map.o Release/obj.target/libsass/src/libsass/src/to_c.o Release/obj.target/libsass/src/libsass/src/to_value.o Release/obj.target/libsass/src/libsass/src/units.o Release/obj.target/libsass/src/libsass/src/utf8_string.o Release/obj.target/libsass/src/libsass/src/util.o Release/obj.target/libsass/src/libsass/src/values.o
  rm -rf "Release/sass.a" && cp -af "Release/obj.target/src/sass.a" "Release/sass.a"
  c++ '-DNODE_GYP_MODULE_NAME=binding' '-DUSING_UV_SHARED=1' '-DUSING_V8_SHARED=1' '-DV8_DEPRECATION_WARNINGS=1' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-DBUILDING_NODE_EXTENSION' -I/usr/local/include/node -I/usr/local/src -I/usr/local/deps/uv/include -I/usr/local/deps/v8/include -I../node_modules/nan -I../src/libsass/include  -fPIC -pthread -Wall -Wextra -Wno-unused-parameter -m64 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++0x -std=c++0x -MMD -MF ./Release/.deps/Release/obj.target/binding/src/binding.o.d.raw   -c -o Release/obj.target/binding/src/binding.o ../src/binding.cpp
In file included from ../src/binding.cpp:1:
../node_modules/nan/nan.h:48:10: fatal error: 'uv.h' file not found
#include <uv.h>
         ^
1 error generated.
gmake: *** [binding.target.mk:115: Release/obj.target/binding/src/binding.o] Błąd 1

Bundled libsass C++ files get compiled but <uv.h> cannot be found when comping node binding file.

@saper
Copy link
Contributor Author

saper commented Sep 1, 2017

@bnoordhuis I need solution to this problem quite urgently, do you think this is a way to go forward? Can you (or anyone) think of better alternatives? (I agree that introduction of an extra file is a kind of overkill)

@rvagg
Copy link
Member

rvagg commented Jun 21, 2019

@saper do you want to continue pursuing this? if so, rebase to master and we'll try and get Ben back in here.

@saper
Copy link
Contributor Author

saper commented Jun 21, 2019

Yes, definitely, this is the #1 reason why plain compilation of binary extensions fails on FreeBSD - uv.h cannot be found.

Node can be compiled with external shared libraries;
in this case append their compile and linker flags last.
@rvagg
Copy link
Member

rvagg commented Jun 22, 2019

@saper can you start considering how a test (or tests) for this might be constructed? There's a few more examples inside test/ these days that you could lean on.

@mscdex
Copy link
Contributor

mscdex commented Mar 15, 2022

While I can't review the proposed changes here, I want to voice my support for this in general because this is becoming a huge pain for addon developers.

Most recently the issue is that some OSes are still building node v17.x (which now bundles OpenSSL 3.x) against a shared OpenSSL 1.x. In this situation, when node-gyp builds, it downloads and builds against the bundled OpenSSL 3.x headers instead of the shared headers, which (depending on OpenSSL API changes) may still allow compilation to complete successfully but will end up causing runtime errors (e.g. due to missing symbols), which is more frustrating than not being able to compile at all because the user thinks the addon is going to work properly after installation.

Some example OSes that are currently doing this include: macOS (specifically homebrew), ArchLinux (and any of its derivatives -- e.g. Manjaro), and some other Linux distro (RH/CentOS-based?) I can't recall offhand right now.

@cclauss
Copy link
Contributor

cclauss commented Mar 15, 2022

Rebase, please?

@mscdex
Copy link
Contributor

mscdex commented Mar 15, 2022

Actually, I just realized that in the case of OpenSSL, I don't think this kind of change will help because of how the headers tarball is structured. Specifically the issue is the include/node in the include_dirs (in addon.gypi). include/node is needed so addons can include node.h and the like, however it's also what makes the compiler look in the include/node/openssl directory when addons do #include <openssl/foo.h>.

At least if the OpenSSL subdirectory was in a separate directory (e.g. include/_openssl, which should never interfere with a #include <...>), addons could conditionally exclude that directory, which would allow shared OpenSSL headers to be used.

@saper
Copy link
Contributor Author

saper commented Mar 27, 2022

At least if the OpenSSL subdirectory was in a separate directory (e.g. include/_openssl, which should never interfere with a #include <...>), addons could conditionally exclude that directory, which would allow shared OpenSSL headers to be used.

This is really bad, do you think node should bundle OpenSSL differently?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants