lz4 dependency causes intermittent build failures

haggholm commented 1 month ago

Originally shared in https://github.com/databricks/databricks-sql-nodejs/issues/245, but that issue was resolved by making the dependency optional (with reduced functionality if omitted). However, lz4 can cause install problems in another way on systems that do have all the dependencies, s.t. NPM attempts to build it, but fails. We have a CI pipeline that usually passes but occasionally fails. We have not yet invested any time into investigating this, but I'm sharing it as a kind of warning sign.

It's worth noting that the LZ4 repository

has a bunch of open issues around install problems, and
has not been active in 4 years, and was recently archived so it's a dependency that's both known to be broken and unlikely to be fixed.

95.47 npm ERR! code 1
95.47 npm ERR! path /home/node/app/node_modules/lz4
95.48 npm ERR! command failed
95.48 npm ERR! command sh -c node-gyp rebuild
95.48 npm ERR! make: Entering directory '/home/node/app/node_modules/lz4/build'
95.48 npm ERR!   CXX(target) Release/obj.target/lz4/lib/binding/lz4_binding.o
95.48 npm ERR! make: Leaving directory '/home/node/app/node_modules/lz4/build'
95.48 npm ERR! gyp info it worked if it ends with ok
95.48 npm ERR! gyp info using node-gyp@10.0.1
95.48 npm ERR! gyp info using node@20.12.2 | linux | x64
95.48 npm ERR! gyp info find Python using Python version 3.7.3 found at "/usr/bin/python3"
95.48 npm ERR! gyp http GET https://nodejs.org/download/release/v20.12.2/node-v20.12.2-headers.tar.gz
95.48 npm ERR! gyp http 200 https://nodejs.org/download/release/v20.12.2/node-v20.12.2-headers.tar.gz
95.48 npm ERR! gyp http GET https://nodejs.org/download/release/v20.12.2/SHASUMS256.txt
95.48 npm ERR! gyp http 200 https://nodejs.org/download/release/v20.12.2/SHASUMS256.txt
95.48 npm ERR! gyp info spawn /usr/bin/python3
95.48 npm ERR! gyp info spawn args [
95.48 npm ERR! gyp info spawn args '/home/node/app/node_modules/node-gyp/gyp/gyp_main.py',
95.48 npm ERR! gyp info spawn args 'binding.gyp',
95.48 npm ERR! gyp info spawn args '-f',
95.48 npm ERR! gyp info spawn args 'make',
95.48 npm ERR! gyp info spawn args '-I',
95.48 npm ERR! gyp info spawn args '/home/node/app/node_modules/lz4/build/config.gypi',
95.48 npm ERR! gyp info spawn args '-I',
95.48 npm ERR! gyp info spawn args '/home/node/app/node_modules/node-gyp/addon.gypi',
95.48 npm ERR! gyp info spawn args '-I',
95.48 npm ERR! gyp info spawn args '/root/.cache/node-gyp/20.12.2/include/node/common.gypi',
95.48 npm ERR! gyp info spawn args '-Dlibrary=shared_library',
95.48 npm ERR! gyp info spawn args '-Dvisibility=default',
95.48 npm ERR! gyp info spawn args '-Dnode_root_dir=/root/.cache/node-gyp/20.12.2',
95.48 npm ERR! gyp info spawn args '-Dnode_gyp_dir=/home/node/app/node_modules/node-gyp',
95.48 npm ERR! gyp info spawn args '-Dnode_lib_file=/root/.cache/node-gyp/20.12.2/<(target_arch)/node.lib',
95.48 npm ERR! gyp info spawn args '-Dmodule_root_dir=/home/node/app/node_modules/lz4',
95.48 npm ERR! gyp info spawn args '-Dnode_engine=v8',
95.48 npm ERR! gyp info spawn args '--depth=.',
95.48 npm ERR! gyp info spawn args '--no-parallel',
95.48 npm ERR! gyp info spawn args '--generator-output',
95.48 npm ERR! gyp info spawn args 'build',
95.48 npm ERR! gyp info spawn args '-Goutput_dir=.'
95.48 npm ERR! gyp info spawn args ]
95.48 npm ERR! gyp info spawn make
95.48 npm ERR! gyp info spawn args [ 'BUILDTYPE=Release', '-C', 'build' ]
95.48 npm ERR! In file included from /root/.cache/node-gyp/20.12.2/include/node/v8.h:29,
95.48 npm ERR!                  from /root/.cache/node-gyp/20.12.2/include/node/node.h:73,
95.48 npm ERR!                  from ../lib/binding/lz4_binding.cc:4:
95.48 npm ERR! /root/.cache/node-gyp/20.12.2/include/node/v8-debug.h:26:3: error: 'Location' does not name a type; did you mean 'Function'?
95.48 npm ERR!    Location GetLocation() const;
95.48 npm ERR!    ^~~~~~~~
95.48 npm ERR!    Function
95.48 npm ERR! /root/.cache/node-gyp/20.12.2/include/node/v8-debug.h: In member function 'int v8::StackFrame::GetLineNumber() const':
95.48 npm ERR! /root/.cache/node-gyp/20.12.2/include/node/v8-debug.h:34:38: error: 'GetLocation' was not declared in this scope
95.48 npm ERR!    int GetLineNumber() const { return GetLocation().GetLineNumber() + 1; }
95.48 npm ERR!                                       ^~~~~~~~~~~
95.48 npm ERR! /root/.cache/node-gyp/20.12.2/include/node/v8-debug.h: In member function 'int v8::StackFrame::GetColumn() const':
95.48 npm ERR! /root/.cache/node-gyp/20.12.2/include/node/v8-debug.h:43:34: error: 'GetLocation' was not declared in this scope
95.48 npm ERR!    int GetColumn() const { return GetLocation().GetColumnNumber() + 1; }
95.48 npm ERR!                                   ^~~~~~~~~~~
95.48 npm ERR! ../lib/binding/lz4_binding.cc: In function 'Nan::NAN_METHOD_RETURN_TYPE LZ4Uncompress_fast(Nan::NAN_METHOD_ARGS_TYPE)':
95.48 npm ERR! ../lib/binding/lz4_binding.cc:364:85: warning: 'int LZ4_decompress_fast(const char*, char*, int)' is deprecated: This function is deprecated and unsafe. Consider using LZ4_decompress_safe() instead [-Wdeprecated-declarations]
95.48 npm ERR!                                                                Buffer::Length(output))
95.48 npm ERR!                                                                                      ^
95.48 npm ERR! In file included from ../lib/binding/lz4_binding.cc:8:
95.48 npm ERR! ../lib/binding/../../deps/lz4/lib/lz4.h:744:16: note: declared here
95.48 npm ERR!  LZ4LIB_API int LZ4_decompress_fast (const char* src, char* dst, int originalSize);
95.48 npm ERR!                 ^~~~~~~~~~~~~~~~~~~
95.48 npm ERR! ../lib/binding/lz4_binding.cc:364:85: warning: 'int LZ4_decompress_fast(const char*, char*, int)' is deprecated: This function is deprecated and unsafe. Consider using LZ4_decompress_safe() instead [-Wdeprecated-declarations]
95.48 npm ERR!                                                                Buffer::Length(output))
95.48 npm ERR!                                                                                      ^
95.48 npm ERR! In file included from ../lib/binding/lz4_binding.cc:8:
95.48 npm ERR! ../lib/binding/../../deps/lz4/lib/lz4.h:744:16: note: declared here
95.48 npm ERR!  LZ4LIB_API int LZ4_decompress_fast (const char* src, char* dst, int originalSize);
95.48 npm ERR!                 ^~~~~~~~~~~~~~~~~~~
95.48 npm ERR! ../lib/binding/lz4_binding.cc: In function 'Nan::NAN_METHOD_RETURN_TYPE LZ4Uncompress(Nan::NAN_METHOD_ARGS_TYPE)':
95.48 npm ERR! ../lib/binding/lz4_binding.cc:333:67: warning: this statement may fall through [-Wimplicit-fallthrough=]
95.48 npm ERR!      sIdx = info[2]->Uint32Value(Nan::GetCurrentContext()).FromJust();
95.48 npm ERR!             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
95.48 npm ERR! ../lib/binding/lz4_binding.cc:334:3: note: here
95.48 npm ERR!    case 2:
95.48 npm ERR!    ^~~~
95.48 npm ERR! make: *** [lz4.target.mk:112: Release/obj.target/lz4/lib/binding/lz4_binding.o] Error 1
95.48 npm ERR! gyp ERR! build error 
95.48 npm ERR! gyp ERR! stack Error: `make` failed with exit code: 2
95.48 npm ERR! gyp ERR! stack at ChildProcess.<anonymous> (/home/node/app/node_modules/node-gyp/lib/build.js:209:23)
95.48 npm ERR! gyp ERR! System Linux 6.2.0-1018-aws
95.48 npm ERR! gyp ERR! command "/usr/local/bin/node" "/home/node/app/node_modules/.bin/node-gyp" "rebuild"
95.48 npm ERR! gyp ERR! cwd /home/node/app/node_modules/lz4
95.48 npm ERR! gyp ERR! node -v v20.12.2
95.48 npm ERR! gyp ERR! node-gyp -v v10.0.1
95.48 npm ERR! gyp ERR! not ok

gabegorelick commented 1 week ago

Is the lz4 dependency truly optional? Will the databricks client work without it?

haggholm commented 1 week ago

Is the lz4 dependency truly optional? Will the databricks client work without it?

It doesn't look truly optional to me—I don't have a repro case for this, nor do I know under exactly what conditions it's significant; but from inspection it looks like the client will just fail if it gets an LZ4 compressed response but the lz4 module isn't available: https://github.com/databricks/databricks-sql-nodejs/blob/56bd0d90d61a2bdff4e62cd55ebcb8d070fa60e2/lib/result/ArrowResultHandler.ts#L29-L31

That doesn't feel "truly optional"—it would be one thing if it gracefully fell back to a less performant JS implementation or something, but…

kravets-levko commented 1 week ago

@haggholm the fragment you shared is intended to show meaningful error if something went really bad. In fact, lz4 is imported conditionally here https://github.com/databricks/databricks-sql-nodejs/blob/56bd0d90d61a2bdff4e62cd55ebcb8d070fa60e2/lib/utils/lz4.ts#L1-L16

Then, server is asked to return compressed results only if lz4 module is available: https://github.com/databricks/databricks-sql-nodejs/blob/56bd0d90d61a2bdff4e62cd55ebcb8d070fa60e2/lib/DBSQLSession.ts#L205

databricks / databricks-sql-nodejs

lz4 dependency causes intermittent build failures #266