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

build: missing defenitions from headers tarball #23167

Closed
refack opened this issue Sep 29, 2018 · 3 comments · Fixed by #23426
Closed

build: missing defenitions from headers tarball #23167

refack opened this issue Sep 29, 2018 · 3 comments · Fixed by #23426
Labels
build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency. question Issues that look for answers. v8 engine Issues and PRs related to the V8 dependency. zlib Issues and PRs related to the zlib subsystem.

Comments

@refack
Copy link
Contributor

refack commented Sep 29, 2018

  • Version:
  • Platform:
  • Subsystem:

I did a semi-thorough grep for symbols our deps use to customise their build, and found a few. First one might have helped find #23122 earlier.
Question is, should we define them, and how (probably via common.gypi)?

  • v8config.h
#if defined(V8_DEPRECATION_WARNINGS) && V8_HAS_ATTRIBUTE_DEPRECATED_MESSAGE
#if defined(V8_IMMINENT_DEPRECATION_WARNINGS) && \
  • zlib\zconf.h
#if defined(ZLIB_CONST) && !defined(z_const)
\include\node\openssl\zlib.h
#ifdef HAVE_UNISTD_H    /* may be set to #if 1 by ./configure */
#  define Z_HAVE_UNISTD_H
#endif
#ifdef HAVE_STDARG_H    /* may be set to #if 1 by ./configure */
#  define Z_HAVE_STDARG_H
#endif
  • openssl
# if defined(OPENSSL_NO_STDIO)
#if defined(OPENSSL_NO_ASM)
# if defined(OPENSSL_THREADS) && !defined(CRYPTO_TDEBUG)
# elif defined(OPENSSL_SYS_WINDOWS) && defined(OPENSSL_OPT_WINDLL)

@nodejs/build-files

@refack refack added question Issues that look for answers. zlib Issues and PRs related to the zlib subsystem. build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. openssl Issues and PRs related to the OpenSSL dependency. labels Sep 29, 2018
@addaleax
Copy link
Member

The zlib symbols should all be taken care of – we have const available, HAVE_STDARG_H is not used for the features we need in core, and Z_HAVE_UNISTD_H is already set in deps/zlib/zlib.gyp.

OPENSSL_NO_ASM is provided by our own configure script’s --openssl-no-asm.
OPENSSL_THREADS sounds like something we don’t need because we already provide threadpool-based crypto functions.

@refack
Copy link
Contributor Author

refack commented Sep 30, 2018

The problem I see is that our configuration and gyp files are not necessarily propogated to the headers tarball.

@addaleax
Copy link
Member

The problem I see is that our configuration and gyp files are not necessarily propogated to the headers tarball.

I’m not sure but I don’t think it’s an issue – the only thing that could potentially be defined is OPENSSL_NO_ASM, but that shouldn’t affect ABI/API, so it’s okay to have diverging definitions (and probably doesn’t have any impact at all on addons)?

@refack refack self-assigned this Oct 4, 2018
refack added a commit to refack/node that referenced this issue Nov 5, 2018
* `V8_DEPRECATION_WARNINGS` is here for explicity. It is already defined
  in `node.gypi`, and `addon.gypi`.
* `V8_IMMINENT_DEPRECATION_WARNINGS` added to warn addons authors.
* `OPENSSL_THREADS` apears in the openSSL `.h` files, and was only
  defined via GYP for the node build.

PR-URL: nodejs#23426
Fixes: nodejs#23167
Refs: nodejs#23122
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@refack refack removed their assignment Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency. question Issues that look for answers. v8 engine Issues and PRs related to the V8 dependency. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants