NAPI_VERSION redefined

[gabrielschulhof]

Version

main

Platform

all

Subsystem

node-api

What steps will reproduce the bug?

  c++ -o /Users/gschulhof/dev/node/out/Release/obj.target/cctest/test/cctest/test_node_crypto.o ../test/cctest/test_node_crypto.cc '-DV8_DEPRECATION_WARNINGS' '-DV8_IMMINENT_DEPRECATION_WARNINGS' '-D_GLIBCXX_USE_CXX11_ABI=1' '-DNODE_OPENSSL_CONF_NAME=nodejs_conf' '-DNODE_OPENSSL_HAS_QUIC' '-DICU_NO_USER_DATA_OVERRIDE' '-D_DARWIN_USE_64_BIT_INODE=1' '-DOPENSSL_NO_PINSHARED' '-DOPENSSL_THREADS' '-DNODE_ARCH="arm64"' '-DNODE_WANT_INTERNALS=1' '-DHAVE_OPENSSL=1' '-DHAVE_INSPECTOR=1' '-D__POSIX__' '-DNODE_USE_V8_PLATFORM=1' '-DNODE_V8_SHARED_RO_HEAP' '-DNODE_HAVE_I18N_SUPPORT=1' '-DNODE_PLATFORM="darwin"' '-DOPENSSL_API_COMPAT=0x10100000L' '-DGTEST_HAS_POSIX_RE=0' '-DGTEST_LANG_CXX11=1' '-DBASE64_STATIC_DEFINE' '-DUNIT_TEST' '-DUCONFIG_NO_SERVICE=1' '-DU_ENABLE_DYLOAD=0' '-DU_STATIC_IMPLEMENTATION=1' '-DU_HAVE_STD_STRING=1' '-DUCONFIG_NO_BREAK_ITERATION=0' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-DNGHTTP2_STATICLIB' '-DNDEBUG' '-DL_ENDIAN' '-DOPENSSL_BUILDING_OPENSSL' '-DECP_NISTZ256_ASM' '-DKECCAK1600_ASM' '-DOPENSSL_BN_ASM_MONT' '-DOPENSSL_CPUID_OBJ' '-DPOLY1305_ASM' '-DSHA1_ASM' '-DSHA256_ASM' '-DSHA512_ASM' '-DVPAES_ASM' '-DOPENSSL_PIC' '-DNGTCP2_STATICLIB' '-DNGHTTP3_STATICLIB' -I../src -I../tools/msvs/genfiles -I../deps/v8/include -I../deps/cares/include -I../deps/uv/include -I../deps/uvwasi/include -I../test/cctest -I../deps/base64/base64/include -I../deps/googletest/include -I../deps/histogram/src -I../deps/histogram/include -I../deps/simdutf -I../deps/ada -I../deps/icu-small/source/i18n -I../deps/icu-small/source/common -I../deps/zlib -I../deps/llhttp/include -I../deps/nghttp2/lib/includes -I../deps/brotli/c/include -I../deps/openssl/openssl/include -I../deps/openssl/openssl/crypto/include -I../deps/openssl/config/archs/darwin64-arm64-cc/asm_avx2/include -I../deps/openssl/config/archs/darwin64-arm64-cc/asm_avx2 -I../deps/ngtcp2 -I../deps/ngtcp2/ngtcp2/lib/includes -I../deps/ngtcp2/ngtcp2/crypto/includes -I../deps/ngtcp2/ngtcp2/crypto -I../deps/ngtcp2/nghttp3/lib/includes  -O3 -gdwarf-2 -mmacosx-version-min=10.15 -arch arm64 -Wall -Wendif-labels -W -Wno-unused-parameter -Werror=undefined-inline -Werror=extra-semi -Wall -Wendif-labels -W -Wno-unused-parameter -std=gnu++17 -stdlib=libc++ -fno-rtti -fno-exceptions -fno-strict-aliasing -MMD -MF /Users/gschulhof/dev/node/out/Release/.deps//Users/gschulhof/dev/node/out/Release/obj.target/cctest/test/cctest/test_node_crypto.o.d.raw   -c
In file included from ../test/cctest/test_linked_binding.cc:2:
In file included from ../test/cctest/node_test_fixture.h:7:
In file included from ../src/node.h:76:
../src/node_version.h:96:9: warning: 'NAPI_VERSION' macro redefined [-Wmacro-redefined]
#define NAPI_VERSION 9
        ^
../src/js_native_api.h:20:9: note: previous definition is here
#define NAPI_VERSION 8
        ^

We need to figure out the sequence of include files. We want to avoid such warnings.

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

No such warnings.

What do you see instead?

This warning.

Additional information

No response

[mhdawson]

@legendecas this is likely a result of the recent addition of NAPI_VERSION 9, possibly we have something missing in our doc of the steps needed for a release.

[mhdawson]

We may need an additional define MAX_NODE_API_VERSION which we use when reporting what node version is supported.

[legendecas]

@mhdawson AFAICT, this only happens when node_version.h is included after js_native_api.h, which wouldn't be the case for Node-API add-ons since node_version.h is not part of Node-API headers.

We may need an additional define MAX_NODE_API_VERSION which we use when reporting what node version is supported.

Are you referring to something like napi_get_version()?

[mhdawson]

napi_get_version()?

correct but maybe you already handled that some other way?

[mhdawson]

@mhdawson AFAICT, this only happens when node_version.h is included after js_native_api.h, which wouldn't be the case for Node-API add-ons since node_version.h is not part of Node-API headers.

So if I understand correctly this is only an issue for our internal testing?

[legendecas]

So if I understand correctly this is only an issue for our internal testing?

It would be an issue for embedder's linked node-api addons if node.h is included after js_native_api.h.

[mhdawson]

@legendecas still trying to understrand the scope of the concern.

So maybe a stupid question but what if people simply include node.h before js_native_api.h. Is there no issue or the reverse issue. If it's the latter it seems like we may need to look at what NAPI_VERSION is being used in within core and possibly have a new define which is used in the cases that currently need the value to be set to NAPI_VERSION 9. That's why I mentioned possibly introducing a MAX_NODE_API_VERSION which as you say is likely what should be returned by napi_get_version() even if an addon author has defined NAPI_VERSION as something else.

[legendecas]

@mhdawson There would be no warnings if node.h is included before js_native_api.h.

Indeed, the NAPI_VERSION defined in node_version.h has a different meaning than the one defined in the addons. The one from node_version.h is identical to napi_get_version(), indicating the highest Node-API version supported by the Node.js runtime. While the NAPI_VERSION defined in addons indicates the Node-API version required by the addon and should be lower than or equal to the NAPI_VERSION of the Node.js runtime.

As node_version.h is a public header file, renaming the macro NAPI_VERSION can be breaking. If we introduce a new MAX_NODE_API_VERSION without renaming the NAPI_VERSION defined in node_version.h, I'm afraid the conflict still preserves.

[mhdawson]

@legendecas

NAPI_VERSION can be breaking

to addons or something else?

If it is something else and there is a relatively low likelyhood then maybe we can change it and mark SemVer major?

[legendecas]

node_version.h is not part of node-api headers -- so it's not breaking node-api addons. For embedders and C++ addons, marking the change as semver-major makes sense to me.

[mhdawson]

I don't think non node-api C++ addons should be using NAPI_VERSION ?

[legendecas]

yeah, but it's there and can be used. Though I don't think it would be meaningful to be used.

[mhdawson]

@legendecas thanks for confirming. I think give our discussion the risk of breakage to any addon is quite low, and should not affect Node-api addons where the ABI guarantee exists. Therefore, I think that the way to go is to rename and mark the PR as SemVer major.

It should have no effect on node-api addons, very small chance of affecting any NaN/other addons and being SemVer major should cover any impact on embedders if any.

[legendecas]

Reopening to wait for https://github.com/nodejs/node/pull/48501.

[legendecas]

Fixed in https://github.com/nodejs/node/pull/48501.