fix: resolve paths used in the node-gyp-build fn #168
Conversation
Ethan-Arrowood
left a comment
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
As long as CI is green I'm okay landing this. I don't think we require a new unit test here but if others disagree we should definitely add one!
|
I think a new unit test is required here. Please add one covering the case you're fixing and then this will be good to go |
|
Test added, works locally 👍 I think it needs a button-push to run here |
|
Looks like it needs a linux and a windows output. Not sure how other tests accomplish this. |
Oh that is annoying 🤔 I can try figure out how to do this, maybe platform specific input/output files |
|
Yeah unfortunately I am not familiar with this repo or I'd offer to help more with the tests. The maintainers who know how this all works are on vacation at the moment 😅 |
|
is this not going to be merged? |
| const os = __webpack_require__(87); | ||
| const path = __webpack_require__(622); | ||
|
|
||
| exports = module.exports = require(__webpack_require__.ab + "prebuilds/darwin-x64/node.napi.node"); |
There was a problem hiding this comment.
Looks like this test is failing on windows and linux because this is the result when running on macos.
You might need to do a find/replace on the output here before running the test to make sure it works on all platforms.
This is based on existing PR #168 by @MarshallOfSound I rebased the original commits except for the one introducing the tests which I have rewritten Instead of requiring an external library (ffi-napi), I created a dummy binding that node-pre-gyp can use as input with a predictable binding file path that won't depend on the platform. --------- Co-authored-by: Samuel Attard <[email protected]> Co-authored-by: Samuel Attard <[email protected]>
|
Closing since this landed in #185 |
4 participants
This fixes cases where modules do
or something similar, this was the only one where
__dirnamewas required, bindings and friends already do this resolution