avh4 / binwrap

Distribute binaries via npm
39 stars 17 forks source link

feat: Support custom binaries paths #39

Open daviddias opened 5 years ago

daviddias commented 5 years ago

Hi @avh4, this PR fixes https://github.com/avh4/binwrap/issues/38 so that binwrap can find binaries in nested directories.

Let me know if this PR is ok. I followed the codestyle but since there was no contributing.md, I might have missed something.


Note: I was able to successful get it to work with my own binary but I was never successful running the tests in the repo either with master or my fork. The error was:

» npm test

> binwrap@0.2.2 test /Users/imp/code/binwrap
> (cd test_app && ./build_packages.sh) && mocha && eslint .

  binwrap
    1) "before each" hook for "fails when specified URLs don't exist"

  0 passing (298ms)
  1 failing

  1) binwrap
       "before each" hook for "fails when specified URLs don't exist":
     ChildProcessError: Command failed: (cd test_app && npm run-script prepare)
sh: binwrap-prepare: command not found
npm ERR! code ELIFECYCLE
npm ERR! syscall spawn
npm ERR! file sh
npm ERR! errno ENOENT
npm ERR! echoMe@0.0.0 prepare: `binwrap-prepare`
npm ERR! spawn ENOENT
npm ERR!
npm ERR! Failed at the echoMe@0.0.0 prepare script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm WARN Local package.json exists, but node_modules missing, did you mean to install?

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/imp/.npm/_logs/2019-09-12T07_33_05_788Z-debug.log
 `(cd test_app && npm run-script prepare)` (exited with error code 1)
      at callback (node_modules/child-process-promise/lib/index.js:33:27)
      at ChildProcess.exithandler (child_process.js:301:5)
      at maybeClose (internal/child_process.js:982:16)
      at Process.ChildProcess._handle.onexit (internal/child_process.js:259:5)

Update: Apparently it might be something with my setup. Travis runs the tests like a charm https://travis-ci.org/avh4/binwrap/builds/584012655

daviddias commented 5 years ago

Hi @avh4, did you had the chance to look at this PR? Alternatively, can you let me know what should be my expectation in a review/consideration/merge/release? Thank you! :)

daviddias commented 5 years ago

Hi @avh4! I'm pretty sure you are really busy. Let me know when you have a chance to read this PR and if you intent do review it or if you have different plans when it comes to keep supporting this module.

avh4 commented 4 years ago

Hi, sorry for the long delay on a reply!

After reading #38, I think it would be preferable to make binaries allow folder names as you tried in your original attempt rather than leaving that still not working and adding a new configuration option. Besides being a simpler config API, allowing folder paths in binaries would also be more flexible to allow multiple binaries to be in different nested folders.

Lmk if you want to try to do a fix that way (and if not, you should be able to make use of your fork in the meantime either by publishing it with a scope as @<username>/binwrap or by using git urls in your package.json)

daviddias commented 4 years ago

After reading #38, I think it would be preferable to make binaries allow folder names

I might be misunderstanding you, but from what you describe, that is exactly what this PR achieves. Look at https://github.com/ipfs/npm-go-ipfs/pull/24/files for an example of that.

image