Skip to content

Conversation

@daviddias
Copy link

@daviddias daviddias commented Sep 12, 2019

Hi @avh4, this PR fixes #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
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

avh4 commented Nov 15, 2019

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
Copy link
Author

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

Base automatically changed from master to main February 4, 2021 23:54
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

Successfully merging this pull request may close these issues.

Any option to signal that the binary is inside a folder and not the single file of the fetched url?

2 participants