Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Make better-sqlite3 / caching feature an optional dependency #49

Closed
gartz opened this issue Sep 20, 2017 · 8 comments
Closed

Comments

@gartz
Copy link

gartz commented Sep 20, 2017

The latest version added a package better-sqlite3 that depends on integer@1.0.1 that requires python to run a script after install.

Can we keep only JS dependencies, please? It makes easier to be cross-compatible.

@ryan-roemer
Copy link
Member

Can you explain more? My understanding is that better-sqlite3 is fully cross-compatible on all OSes.

python is the basis of https://github.com/nodejs/node-gyp which has been around since forever IIRC as the basis of native bindings building. Are you saying that you'd like us to scrub all potential dependencies to make sure there are no native bindings anywhere?

And as a potentially helpful tip, I'd generally recommend that you never actually install software on your running servers. Just on your CI server, which builds all packages -- then tar that up, archive the artifact, and deploy the artifact to your servers -- to ensure that you never are at the mercy of realtime installs where things could go wrong / need to be shrinkwrap/package-lock'ed, etc.

/cc @tptee

@gartz
Copy link
Author

gartz commented Sep 20, 2017 via email

@ryan-roemer
Copy link
Member

@gartz -- Hmmm... while it does seem awkward that you need to reach out to any project that has any transitive dependency on anything using node-gyp, in our case, we use sqlite as a caching tool, so it's not strictly necessary. If we moved that to optionalDependencies would that work for you?

(For example, chokidar which comes up all the time and I'd be surprised if you didn't have a transitive dependency on with any of the collection of modern JS build tools has an optionalDependencies on fsevents. The fsevents things has come up specifically with dev's trying to do locking on a different OS than the one that's expected -- definitely not a recommended practice 😉 )

@ryan-roemer
Copy link
Member

Driving home the chokidar example more concretely -- it's needed for webpack:

(nvm v8.3.0) rye@small ~/Desktop/temp-wp $ npm install webpack

> fsevents@1.1.2 install /Users/rye/Desktop/temp-wp/node_modules/fsevents
> node install

[fsevents] Success: "/Users/rye/Desktop/temp-wp/node_modules/fsevents/lib/binding/Release/node-v57-darwin-x64/fse.node" already installed
Pass --update-binary to reinstall or --build-from-source to recompile

# ... SNIPPED ...

(nvm v8.3.0) rye@small ~/Desktop/temp-wp $ find node_modules | grep binding.gyp
node_modules/fsevents/binding.gyp

... and I'm strongly intuiting your build uses webpack given you're using this project 😉

@gartz
Copy link
Author

gartz commented Sep 20, 2017 via email

@ryan-roemer
Copy link
Member

Because fsevents is an optionalDepedency in chokidar. That's why I'm proposing that as a potential solution to help just your bespoke CI constraints.

Yarn shows the deps:

chokidar@^1.7.0:
  version "1.7.0"
  resolved "https://registry.yarnpkg.com/chokidar/-/chokidar-1.7.0.tgz#798e689778151c8076b4b360e5edd28cda2bb468"
  dependencies:
    anymatch "^1.3.0"
    async-each "^1.0.0"
    glob-parent "^2.0.0"
    inherits "^2.0.1"
    is-binary-path "^1.0.0"
    is-glob "^2.0.0"
    path-is-absolute "^1.0.0"
    readdirp "^2.0.0"
  optionalDependencies:
    fsevents "^1.0.0"

===> 

watchpack@^1.4.0:
  version "1.4.0"
  resolved "https://registry.yarnpkg.com/watchpack/-/watchpack-1.4.0.tgz#4a1472bcbb952bd0a9bb4036801f954dfb39faac"
  dependencies:
    async "^2.1.2"
    chokidar "^1.7.0"
    graceful-fs "^4.1.2"

===> 

webpack@^3.6.0:
  version "3.6.0"
  resolved "https://registry.yarnpkg.com/webpack/-/webpack-3.6.0.tgz#a89a929fbee205d35a4fa2cc487be9cbec8898bc"
  dependencies:
    acorn "^5.0.0"
    acorn-dynamic-import "^2.0.0"
    ajv "^5.1.5"
    ajv-keywords "^2.0.0"
    async "^2.1.2"
    enhanced-resolve "^3.4.0"
    escope "^3.6.0"
    interpret "^1.0.0"
    json-loader "^0.5.4"
    json5 "^0.5.1"
    loader-runner "^2.3.0"
    loader-utils "^1.1.0"
    memory-fs "~0.4.1"
    mkdirp "~0.5.0"
    node-libs-browser "^2.0.0"
    source-map "^0.5.3"
    supports-color "^4.2.1"
    tapable "^0.2.7"
    uglifyjs-webpack-plugin "^0.4.6"
    watchpack "^1.4.0"
    webpack-sources "^1.0.1"
    yargs "^8.0.2"

So if you're planning on using and eventually upgrading webpack to this one, you'll have an optionaldep on fsevents. Honestly, I'm guessing that has been there a very, very long time.

@ryan-roemer
Copy link
Member

ryan-roemer commented Sep 20, 2017

And fsevents is not part of the inspectpack dependency tree:

$ yarn add inspectpack
$ find node_modules | grep binding.gyp
node_modules/better-sqlite3/binding.gyp
node_modules/farmhash/binding.gyp
node_modules/integer/binding.gyp
$ cat yarn.lock | grep fsevents
# ... NO OUTPUT ...

Side note: farmhash is already an optionalDependencies

@gartz
Copy link
Author

gartz commented Sep 21, 2017

Indeed actually there are other dependencies that include fsevents as optional and it is present in my package-lock.json. However they don't fail during the build. Let me show you this:

npm info lifecycle integer@1.0.1~install: integer@1.0.1

> integer@1.0.1 install /code/node_modules/integer
> node tools/install

gyp info it worked if it ends with ok
gyp info using node-gyp@3.6.2
gyp info using node@8.5.0 | linux | x64
gyp ERR! configure error
gyp ERR! stack Error: Can't find Python executable "python", you can set the PYTHON env variable.
gyp ERR! stack     at PythonFinder.failNoPython (/usr/local/lib/node_modules/npm/node_modules/node-gyp/lib/configure.js:483:19)
gyp ERR! stack     at PythonFinder.<anonymous> (/usr/local/lib/node_modules/npm/node_modules/node-gyp/lib/configure.js:397:16)
gyp ERR! stack     at F (/usr/local/lib/node_modules/npm/node_modules/which/which.js:68:16)
gyp ERR! stack     at E (/usr/local/lib/node_modules/npm/node_modules/which/which.js:80:29)
gyp ERR! stack     at /usr/local/lib/node_modules/npm/node_modules/which/which.js:89:16
gyp ERR! stack     at /usr/local/lib/node_modules/npm/node_modules/which/node_modules/isexe/index.js:42:5
gyp ERR! stack     at /usr/local/lib/node_modules/npm/node_modules/which/node_modules/isexe/mode.js:8:5
gyp ERR! stack     at FSReqWrap.oncomplete (fs.js:153:21)
gyp ERR! System Linux 4.9.49-moby
gyp ERR! command "/usr/local/bin/node" "/usr/local/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /code/node_modules/integer
gyp ERR! node -v v8.5.0
gyp ERR! node-gyp -v v3.6.2
gyp ERR! not ok
npm info lifecycle integer@1.0.1~install: Failed to exec install script
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.1.2 (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.1.2: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! integer@1.0.1 install: `node tools/install`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the integer@1.0.1 install script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

This is what happens after:

npm install webpack-dashboard@next --save-dev

Now let's see without it by running:

npm uninstall webpack-dashboard --save-dev

To be sure the only thing I removed was this project, because inspectpack is the dependency that is breaking webpack-dashboard latest version.

npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.1.2 (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.1.2: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})

added 1814 packages in 24.598s
npm info ok

Unsupported dependencies are trying to install in the linux build. No errors or warnings about the node-gyp yes it's a webpack project.

fsevents is in my package-lock.json, but kcheck this out:

    "fsevents": {
      "version": "1.1.2",
      "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-1.1.2.tgz",
      "integrity": "sha512-Sn44E5wQW4bTHXvQmvSHwqbuiXtduD6Rrjm2ZtUEGbyrig+nUH3t/QD4M4/ZXViY556TBpRgZkHLDx3JxPwxiw==",
      "dev": true,
      "optional": true,
      "requires": {
        "nan": "2.7.0",
        "node-pre-gyp": "0.6.36"
      },

As you mentioned, it is an optional, so no conflicts.

I think that if you move those dependencies to optional, it would be great because I want to install in my OS-X but I don't need them to build the project in my controlled build environment on the CI server.

Thank you for checking that and inspect all those packages. This is a great project and I like to use it if I can.

@ryan-roemer ryan-roemer changed the title Can't install in servers without python. Feature: Make better-sqlite3 / caching feature an optional dependency Sep 26, 2017
ryan-roemer added a commit that referenced this issue Sep 29, 2017
* Make `better-sqlite3` and `optionalDependency`. Switch to noop cache if not present. Fixes #49
* Add `Cache.wrapAction` helper for common use case of "try cache get, do action, set cache".
* Change cross-process communication to just serialize/deserialize the applicable cache instance.
* Various refactoring and cleanup.
* Re-enabled eslint rules for better hamonized style.
* Add `cache` option for `InspectpackDaemon.create`.
* Add error logging for worker errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants