Better error msg when Instant Client is missing #404

bchr02 · 2016-04-08T22:28:39Z

If there is a pending error and Instant Client is not found in PATH then an error message is displayed prior to throw err;.

Otherwise, if it is found in PATH then the directory it is first found in is outputed with a message asking the user to check that the correct version is installed.

Signed-off-by: Bill Christo [email protected]

cjbj · 2016-04-08T23:33:17Z

lib/oracledb.js

+    process.env.PATH.split(path.delimiter).forEach(function(dir) {
+      if(existsSync(path.join(dir, 'oci.dll'))) {
+		  console.log();
+		  console.log('**** Found Oracle Instant Client in PATH in the following direcory:' + dir);


Good idea to improve this. Can the messages cater for users with ORACLE_HOME installs too? Something like "Found the Oracle client library ..." Testing PATH will only work on Windows; I'd be OK making the improved validation work on Windows-only for the moment.

Yes, I will modify to cater to ORACLE_HOME too. Good idea. Also, for Linux we could search for libclntsh.so.12.1 or libclntsh.so.11.1, right?

For non-Windows I think you'd be OK checking for 'libclntsh.*' Harder to portably check the link has been created.

This doesn't look right:

path = require('path'); existsSync = require('fs').existsSync || require('path').existsSync;

I can't find existsSync in path. Also, existsSync is deprecated:
https://nodejs.org/api/fs.html#fs_fs_existssync_path

The main answer here offers an alternative:
http://stackoverflow.com/questions/4482686/check-synchronously-if-file-directory-exists-in-node-js

Yes, you're right. I will update it.

sent from iPhone.

On Apr 9, 2016, at 11:20 AM, Dan McGhan [email protected] wrote:

In lib/oracledb.js:

try {
oracledbCLib = require('../build/Release/oracledb');
} catch (err) {
if (err.code === 'MODULE_NOT_FOUND') {
oracledbCLib = require('../build/Debug/oracledb');
} else {

path = require('path');

existsSync = require('fs').existsSync || require('path').existsSync;

process.env.PATH.split(path.delimiter).forEach(function(dir) {

if(existsSync(path.join(dir, 'oci.dll'))) {

console.log();

console.log('***\* Found Oracle Instant Client in PATH in the following direcory:' + dir);
This doesn't look right:

path = require('path'); existsSync = require('fs').existsSync || require('path').existsSync;

I can't find existsSync in path. Also, existsSync is deprecated:
https://nodejs.org/api/fs.html#fs_fs_existssync_path

This the main answer here offers an alternative:
http://stackoverflow.com/questions/4482686/check-synchronously-if-file-directory-exists-in-node-js

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

sagiegurari · 2016-04-09T06:35:23Z

I recommend not to use console.log for valid things, instead to use https://www.npmjs.com/package/debuglog
Otherwise users will always see messages without a way to disable them.

bchr02 · 2016-04-09T13:14:17Z

@sagiegurari isn't it kinda of a moot point since we are throwing an error? But while we're on the topic shouldn't we be handling the errors asynchronously by passing the error up the callback chain instead of try/catching it?

sagiegurari · 2016-04-09T13:35:36Z

error you say? you have printouts to console.log for valid scenarios.
so either remove it, or use debuglog

 if(existsSync(path.join(dir, 'oci.dll'))) {
          console.log();
          console.log('**** Found Oracle Instant Client in PATH in the following direcory:' + dir);

bchr02 · 2016-04-09T13:41:35Z

It's not a valid scenario. This only prints out when it's the wrong instant client version.

sagiegurari · 2016-04-09T13:42:52Z

ok, now i get you. than forget it, you are right.

sagiegurari · 2016-04-09T13:44:51Z

in windows, i saw an issue for people who used node 64 bit with oci 32 bit.
Well... they had multiple ocis (don't ask me why) and it compiled with one and ran with another (again don't ask me why, wasn't my pc but people have a mess) and I had to use dependency walker to see that issue.
I wonder if you can provide that info as well, meaning provide the node 32/64 bit version and the oci 32/64 bit version (if you find the oci).
what do you think?

dmcghan · 2016-04-09T13:48:18Z

Not all errors are asynchronous:
https://www.joyent.com/developers/node/design/errors

This try/catch is only executed once to bring in the C lib when the module is first "required" in. There is no callback chain in this context anyway, so throwing is the correct behavior.

bchr02 · 2016-04-09T14:00:02Z

@dmcghan okay that makes sense then.

@sagiegurari yea i thought about including the node.js version but didn't know how to extrapolate the same information from the dll, so for now I decided to hold off. As far as having multiple instant clients in path, the first one to appear is the one that is used which is why only the first one's directory is displayed. Perhaps the error message should indicate this.

sagiegurari · 2016-04-09T15:46:42Z

maybe this can help:
http://stackoverflow.com/questions/495244/how-can-i-test-a-windows-dll-to-determine-if-it-is-32bit-or-64bit
or
http://stackoverflow.com/questions/197951/how-can-i-determine-for-which-platform-an-executable-is-compiled

These are not js implementations but i'm sure you can convert it to js to validate.
This means reading the dll in certain area and checking for the value in the header if it is 32 or 64 bit.
Didn't find an npm module written for that.
I'm not sure about linux or mac.

bchr02 · 2016-04-09T21:38:24Z

I was just doing some tests on Windows. I didn't have any environment variables set. I opened a command prompt and typed set ORACLE_HOME=c:\oracle\instantclient but oracledb would not work. I had to set the directory in PATH for it to work. Why is that? It's my understanding that ORACLE_HOME takes precedent over PATH.

bchr02 · 2016-04-10T06:51:15Z

In response to everyone's feedback, I just pushed the following changes:

now using statSync to check whether oci.dll exists
added fileMatch function to allow the searching of any files beginning with libclntsh.
added more information to the message that's displayed when an incorrect lib is detected.

I haven't added the scanning of ORACLE_HOME yet. I am waiting to get some feedback from @cjbj on this. Need to better understand when ORACLE_HOME is used and whether it is preferred over PATH. As explained, on Windows having it defined does not appear to influence anything. Maybe it's only for Linux?

kubo · 2016-04-10T08:59:45Z

It's my understanding that ORACLE_HOME takes precedent over PATH.

As for instant client, ORACLE_HOME is referred only when TNS_ADMIN isn't set and it is used to locate configuration files such as tnsnames.ora or sqlnet.ora.

As for full client, ORACLE_HOME must be set. Otherwise Oracle client doesn't work. On Unix ORACLE_HOME must be set as an environment variable. On Windows it is set in the Windows registry.

kubo · 2016-04-10T09:39:54Z

added fileMatch function to allow the searching of any files beginning with libclntsh.

It doesn't work on Unix because the environment variable PATH is not used to locate dependent shared libraries on Unix.

On Linux, the location of libclntsh.so is embedded in the compiled node-oracledb binary by the linker option -Wl,-rpath,/directory/containing/libclntsh.
See https://github.com/oracle/node-oracledb/blob/v1.8.0/binding.gyp#L30
If libclntsh.so is not found in the embedded path, the dynamic loader tries to find libclntsh.so in the environment variable LD_LIBRARY_PATH, directories in /etc/ld.so.conf (/etc/ld.so.cache is created from this file by the ldconfig command.), /lib and /usr/lib.
See http://man7.org/linux/man-pages/man8/ld.so.8.html

On other Unix platforms, similar but different ways are used to locate libclntsh.so.

bchr02 · 2016-04-10T14:06:44Z

Thanks @kubo

As for instant client, ORACLE_HOME is referred only when TNS_ADMIN isn't set and it is used to locate configuration files such as tnsnames.ora or sqlnet.ora.

In my testing on Windows, I did not have TNS_ADMIN set and I can confirm that although I had ORACLE_HOME set it was not used. So I'm still not sure why that is.

In regards to your second reply, based on that I think I may need to make the first section, the one that searches for the library, only for Windows and for everything else just have a standard message explaining that the library was not found. Or perhaps I can search the LD_LIBRARY_PATH?

Before I change anything again, I will wait a bit to give everyone the opportunity to chime in.

kubo · 2016-04-10T15:21:23Z

In my testing on Windows, I did not have TNS_ADMIN set and I can confirm that although I had ORACLE_HOME set it was not used. So I'm still not sure why that is.

ORACLE_HOME is unrelated to the location of OCI.DLL. It is used only to locate network configuration files when TNS_ADMIN is not set in instant client. If you don't use network configuration files such as tnsnames.ora, ORACLE_HOME has no effect.

In regards to your second reply, based on that I think I may need to make the first section, the one that searches for the library, only for Windows and for everything else just have a standard message explaining that the library was not found. Or perhaps I can search the LD_LIBRARY_PATH?

+1 to the former.
It is hard to implement same feature for Unix.

cjbj · 2016-04-11T02:10:03Z

@bchr02 to surface my earlier comments I think it will easier to make the change (for this PR, at least) specific to Windows. The general message terminology needs to use 'Oracle client' instead of 'Oracle Instant Client' since PATH may be using an Oracle Home-style install and not Instant Client.

bchr02 · 2016-04-11T12:59:23Z

I think it will easier to make the change (for this PR, at least) specific to Windows. The general message terminology needs to use 'Oracle client' instead of 'Oracle Instant Client' since PATH may be using an Oracle Home-style install and not Instant Client.

@cjbj Okay, I just pushed those changes. Thanks.

cjbj · 2016-08-19T07:24:49Z

@bchr02 in 1.11 we emit a new NJS error message when the node-oracledb binary can't be loaded but we haven't added any of the investigatory heuristics of this PR.

cjbj · 2017-11-21T03:58:34Z

@bchr02 where do you want to go with this? I adopted the ideas into https://github.com/oracle/node-oracledb/blob/dev-2.0/lib/oracledb.js (thank you!) without the port specific Windows check for oci.dll. Do you want to rework the PR and add it under a platform check for Windows? It should probably mention needing the VS Redistributable. There could be some smarts like looking for a BASIC_README or BASIC_LITE_README to get a version number and then printing out which VS Redistributable is needed?

bchr02 · 2017-11-21T04:46:45Z

@cjbj I would love to. I’ll start working on something.

So, what did you have in mind by looking at the readme’s. Is there a version number within them, some metadata on the files themselves or does the mere fact that they exist mean that VS 2010 is needed over 2005?

cjbj · 2017-11-21T05:07:31Z

@bchr02 If your code finds an oci.dll and there is a BASIC_README or BASIC_LITE_README then you know the user has Instant Client - and the user must separately install a redistributable. You can parse the version out of the readme from the Client Shared Library 64-bit - 12.2.0.1.0 line and then print (from a hardcoded list) which VS redistributable is needed. Or rundumpbin /headers oci.dll and have a lookup table from that number to the VS Redist (see https://github.com/oracle/node-oracledb/blob/v2.0.14-dev/INSTALL.md#--366-install-the-visual-studio-redistributables) - that might be easier now I think of it.

Some of the logic could also be added into https://github.com/oracle/node-oracledb/blob/v2.0.14-dev/package/oracledbinstall.js#L336

These are just brainstorm ideas from a non-windows user.

kubo · 2017-11-21T13:52:53Z

@cjbj Is the getInfo() function in oracledb.js called when OCL.DLL or VS redistributable is missing?

getInfo() is called only when err.code === 'MODULE_NOT_FOUND'. However, as far as I checked node.js source code (Well, I have not checked it by running node.js. Sorry if I mistake), it is set only when a required file itself ("oracledb.node" in this case) is not found.

cjbj · 2017-11-22T01:17:17Z

@kubo Now that ODPI-C uses dlopen() it will pick up some errors first so you're right: the getInfo() call may need to be higher in oracledb.js. The intention was for it to be called when the add-on library (oracledb.node) exists but can't be loaded. I'll do some tweaking.

cjbj · 2017-11-22T02:59:46Z

@kubo. Something like https://gist.github.com/cjbj/78a587855a81edec3fb4dedc8aece522 ?

cjbj · 2017-11-22T21:31:04Z

I updated the gist.

kubo · 2017-11-23T05:59:28Z

@cjbj I checked the gist on Linux. It looks good.

bchr02 · 2017-11-26T02:47:22Z

@cjbj In light of oracle/odpi#48 which route are we going with this? will this be done in ODPI-C or oracledb.js?

cjbj · 2017-11-27T13:35:28Z

@bchr02 I think Anthony will make some changes. Give it a day or two to see what he commits.

cjbj · 2017-12-11T23:48:39Z

The v2.0.15 messages might contain too much information; for example if there is no oracledb.node binary, the messages mention this and then talk about Instant Client. I expect some readers will focus on the wrong parts of the message.

cjbj · 2018-04-03T22:56:28Z

@bchr02 is it time to close this?

bchr02 · 2018-04-03T23:01:04Z

Yes, thank you.

bchr02 added 3 commits April 8, 2016 18:20

Better error msg when Instant Client is missing

71b6143

Better error msg when Instant Client is missing

73d6153

Better error msg when Instant Client is missing

6c29e2e

cjbj reviewed Apr 8, 2016
View reviewed changes

bchr02 added 3 commits April 10, 2016 02:20

Better error msg when Instant Client is missing

3b6bbc7

Better error msg when Instant Client is missing

68d5f8d

Better error msg when Instant Client is missing

10da5cc

Better error msg when Instant Client is missing

270b4d1

Better error msg when Instant Client is missing

8d436ce

cjbj added the oca-verified label Apr 26, 2016

kubo mentioned this pull request Nov 23, 2017

Custom error message when Visual Studio distributable package is not installed on Windows oracle/odpi#48

Open

bchr02 closed this Apr 3, 2018

Better error msg when Instant Client is missing #404

Better error msg when Instant Client is missing #404

Conversation

bchr02 commented Apr 8, 2016

cjbj Apr 8, 2016

Choose a reason for hiding this comment

bchr02 Apr 9, 2016

Choose a reason for hiding this comment

cjbj Apr 9, 2016

Choose a reason for hiding this comment

dmcghan Apr 9, 2016

Choose a reason for hiding this comment

bchr02 Apr 9, 2016

Choose a reason for hiding this comment

try { oracledbCLib = require('../build/Release/oracledb'); } catch (err) { if (err.code === 'MODULE_NOT_FOUND') { oracledbCLib = require('../build/Debug/oracledb'); } else {

sagiegurari commented Apr 9, 2016

bchr02 commented Apr 9, 2016

sagiegurari commented Apr 9, 2016

bchr02 commented Apr 9, 2016

sagiegurari commented Apr 9, 2016

sagiegurari commented Apr 9, 2016

dmcghan commented Apr 9, 2016

bchr02 commented Apr 9, 2016

sagiegurari commented Apr 9, 2016

bchr02 commented Apr 9, 2016

bchr02 commented Apr 10, 2016

kubo commented Apr 10, 2016

kubo commented Apr 10, 2016

bchr02 commented Apr 10, 2016

kubo commented Apr 10, 2016

cjbj commented Apr 11, 2016

bchr02 commented Apr 11, 2016

cjbj commented Aug 19, 2016

cjbj commented Nov 21, 2017

bchr02 commented Nov 21, 2017

cjbj commented Nov 21, 2017

kubo commented Nov 21, 2017

cjbj commented Nov 22, 2017

cjbj commented Nov 22, 2017

cjbj commented Nov 22, 2017

kubo commented Nov 23, 2017

bchr02 commented Nov 26, 2017

cjbj commented Nov 27, 2017

cjbj commented Dec 11, 2017

cjbj commented Apr 3, 2018

bchr02 commented Apr 3, 2018

try {
oracledbCLib = require('../build/Release/oracledb');
} catch (err) {
if (err.code === 'MODULE_NOT_FOUND') {
oracledbCLib = require('../build/Debug/oracledb');
} else {