eclipse-cdt-cloud / cdt-gdb-adapter

CDT GDB Debug Adapter
Eclipse Public License 2.0
31 stars 41 forks source link

Unable to use this adapter as a dependency #149

Closed thegecko closed 4 years ago

thegecko commented 4 years ago

The recent addition of an install script has broken the ability for the adapter to be used as a dependency.

This is because yarn install is automatically called when used as a dependency, but the cross-spawn dependency only exists as a dev-dependency, so isn't found.

If the install script needs to be run even when this package is in production, then cross-spawn needs to be changed to be a full dependency.

If this is only used in development, I recommend the install script is renamed so it isn't run automatically or some other mechanism is used to determine production and cross-spawn isn't always required.

 Command failed.
Exit code: 1
Command: node install.js
Arguments: 
Directory: /Users/thegecko/Projects/cmsis-debug-adapter/node_modules/cdt-gdb-adapter
Output:
internal/modules/cjs/loader.js:638
    throw err;
    ^

Error: Cannot find module 'cross-spawn'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:636:15)
    at Function.Module._load (internal/modules/cjs/loader.js:562:25)
    at Module.require (internal/modules/cjs/loader.js:690:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at Object.<anonymous> (/Users/thegecko/Projects/cmsis-debug-adapter/node_modules/cdt-gdb-adapter/install.js:2:13)
    at Module._compile (internal/modules/cjs/loader.js:776:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:787:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
thegecko commented 4 years ago

cc @jonahgraham

jonahgraham commented 4 years ago

Oops, I guess that my dependent projects all had cross spawn as direct deps, or some other project did. (ohhh, I miss OSGi!)

Yes cross spawn is needed, otherwise the native libraries don't get built. The install.js script sits between yarn install and node-gyp rebuild so that we don't needlessly try to compile natives on Mac and windows where the native library was a no-op.

thegecko commented 4 years ago

Ah, in that case I'd recommend using the built-in node spawn instead of adding another production dependency.

From the readme, it looks like cross-spawn only fixes issues on Windows, but in this case we are only using it on Linux :)

jonahgraham commented 4 years ago

I have pushed a temporary tag so I can test out the flowing flow:

yarn init
yarn install
yarn add cdt-gdb-adapter@bug149

The above (but using @next) is failing, and on Linux it is passing again. Need to try on Windows.

jonahgraham commented 4 years ago

See PR #150

jonahgraham commented 4 years ago

I followed your advice and dropped cross-spawn. I tested on Linux & Windows using the same flow and it is fixed there too. I don't have a Mac, so @thegecko please reopen if it isn't working as expected there.

FIxed in 5638382 and published

thegecko commented 4 years ago

All works well on Mac, thanks for the fix