lz4 dependency causes intermittent build failures

[haggholm]

Originally shared in #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 [email protected]
95.48 npm ERR! gyp info using [email protected] | 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]

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

[haggholm]

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:

databricks-sql-nodejs/lib/result/ArrowResultHandler.ts

Lines 29 to 31 in 56bd0d9

	if (this.isLZ4Compressed && !LZ4) {
	throw new HiveDriverError('Cannot handle LZ4 compressed result: module `lz4` not installed');
	}

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]

@haggholm the fragment you shared is intended to show meaningful error if something went really bad. In fact, lz4 is imported conditionally here

databricks-sql-nodejs/lib/utils/lz4.ts

Lines 1 to 16 in 56bd0d9

	import type LZ4Namespace from 'lz4';
	type LZ4Module = typeof LZ4Namespace;
	function tryLoadLZ4Module(): LZ4Module | undefined {
	try {
	return require('lz4'); // eslint-disable-line global-require
	} catch (err) {
	const isModuleNotFoundError = err instanceof Error && 'code' in err && err.code === 'MODULE_NOT_FOUND';
	if (!isModuleNotFoundError) {
	throw err;
	}
	}
	}
	export default tryLoadLZ4Module();

Then, server is asked to return compressed results only if lz4 module is available:

databricks-sql-nodejs/lib/DBSQLSession.ts

Line 205 in 56bd0d9

canDecompressLZ4Result: clientConfig.useLZ4Compression && Boolean(LZ4),