-
Notifications
You must be signed in to change notification settings - Fork 1k
Bundling sharp doesn't work anymore since updating to 4.5.0 #1075
Comments
have you run |
If the mentioned changes are indeed breaking, I think we would have to revert them immediately and release a new version. First, I would wait for a while for responses from the related parties, and see if there is a misconfiguration or if the changes can be tweaked to be non-breaking. @pkeuter mentioned Any thoughts? @leerob @robertsLando |
Before doing any reverting, would need to verify and confirm it is indeed a breaking change. I'm not convinced those two PRs are related yet but would love to see the investigation here. |
|
I see it is in the package.json and it is also in the package-lock.json (I'm using npm):
and this is top-level the prebuild-install:
This was running in CI so there was no node_modules folder. No cache or something either. When I look in the folder referenced |
@pkeuter ah yes, I see what you mean now. Here's the target reference https://github.com/vercel/pkg/blob/master/lib/producer.js#L100. As you say, If you know how to change the path to handle any combination of situations (local/global, yarn/npm) that would be great. |
@david-mohr I do not know a solution right away, but the fact is that this is, indeed, a breaking change so it might be useful to revert the change in a new release until this has been resolved. Because pkg is currently broken for npm users that use native modules. |
@pkeuter I think this can be patched to work with both @leerob guess it's your call. To fix this, the function can use existsSync to hunt around for the path. Otherwise just back it out, it doesn't seem that other users are interested in cross-platform support, so it's not really a helpful patch. |
I'm sorry you're having this issue @pkeuter |
The prebuild-install problem:The path to the prebuild script is being built incorrectly. The error is in producer.js Line 135 in f08d083
A Workaround:I got it to resolve the correct path by changing that line to: However... The prebuild script shows a warning Manually Installing cross-platform prebuilt binaries:So I installed my native module (sqlite3) manually using Unfortunately:When running pkg I still got the same error: My guess is that the target version was incorrect - but it's hard to know for sure. When I tried adding the So I tried manually added the module as an asset in package.json
When I run my packaged .exe on Windows I can see that the file indeed gets included, but I get this error: So, lastly, I copied the node_sqlite3.node file into So, for some reason, it can't locate the module even though it's right there in the snapshot - but it works when the module is placed alongside it. |
@JeffJassky Usually this kind of problems are fixed by dictionaries. You need to edit this https://github.com/vercel/pkg/blob/master/dictionary/sqlite3.js see an example here: https://github.com/vercel/pkg/blob/master/dictionary/puppeteer.js |
@robertsLando I sort-of understand. These are package-specific configs for pkg - though I don't precisely know what I should put in there - 'cause I don't quite understand what the problem is, to begin with. |
I'm not the author of this package I have just take a general lookup of the code and from what I have understand that's a collection of package-specific fixes. With that you can specify patches (replace/add/remove code from files) and or additional assets/scripts to add for a specific package like: 'use strict';
module.exports = {
pkg: {
scripts: ['lib/middleware/*.js'],
assets: [
'lib/public/**/*', // for [email protected]
],
},
}; In your case I think a possible fix could be adding the assets needed here in the sqlite3 dictionary, something like: 'use strict';
module.exports = {
pkg: {
assets: [
'lib/binding/**/*',
],
},
}; Give this a try and let me know :) |
@robertsLando After looking into it further, the .node module is actually getting included in the snapshot - in the exact correct location. Hmm... the plot thickens. Seems to be related directly to #1075, possibly #894, and #687 |
We might check where the lookup fails so. Based on what I understand native node modules behaves different then normal assets, them are copied in a temporary folder iin order to be used, maybe the lookup or the copy fails somewhere. That feature has been added in #837. Did you alo try to put the node addon in the same root dir of the application to see if that works? The magic happens here: https://github.com/vercel/pkg/pull/837/files#diff-0c3f42fd6825d8d5b4d0422402a89ff11dc1d1cfb52ee5901f4774e503783c27R1598 |
@robertsLando That's exactly what I was thinking it could be. I'll look into your references now. Putting the .node module directly next to the executable didn't work. It only worked when a node_modules folder was placed next to the executable with the .node module nested deeply inside. |
@robertsLando you were right on with the dlopen function. I think I've found the gremlin we're dealing with. The Extended-Length Path Prefix problemIt seems that in some (possibly all) cases, on Windows (and possibly linux?), the paths of manually included assets in the snapshot include "extended length prefixes". For example, the path to my file was: I noticed in the original post by @pkeuter there seemed to be a strange prefix on his path too: Here's someone discussing (for Windows) it on SuperUser: Because of the prefix, it causes this check to fail: Line 1714 in f08d083
making it proceed as if the file doesn't exist in the snapshot, resulting in this error: Then, if you try to actually load the file using the prefix it results in this error: WorkaroundBy stripping the prefix off of the Suggested SolutionAccount for, disable, remove, or otherwise negate extended-length prefixes for manually included assets. |
@JeffJassky Maybe this function could fix the issue? |
Something close to that, yes. Though it seems like the function is assuming a prefix of 4 characters, whereas on my machine it was 5 ( Perhaps you could tweak the function to work for both, when we can both test your branch before PR/merge. |
@JeffJassky By looking again the code I think this has been fixed in #1095 Could you give a try to [email protected] just to be sure? |
@robertsLando @4.5.1 gives me this: which is interesting for a few reasons. The prefix changed, and it moved, and double slashes are gone. |
@JeffJassky Maybe: #1103 ? |
How can I install and test a specific commit like that? |
@JeffJassky You should
Then inside your repo:
|
@robertsLando Identical error to the above: #1075 (comment) I pulled down the branch and made sure to uninstall pkg from my main repo to ensure it built using the locally linked package. It did build - but still has a messed-up path. To clarify, the function seems to be handling all of the extended prefixes with the |
@JeffJassky the sanitize function should also take care of that and replace it too, are you using latest changes from the branch? Could you retry to pull it? |
@robertsLando It's up to date. The function looks like this:
|
@robertsLando The extra **\ stuff is getting added here: Line 188 in cceb78a
I'm having trouble understanding why it won't build in 4.5.1 but I can easily work around the issues in 4.5.0. |
I think that the easier way to fix this for now would be to completely unpublish pkg-fetc 2.6.10 and just work on the 3.0.0 release @leerob |
Is there a recommended workaround for latest version of |
Anyone? What versions of |
latest pkg version |
@robertsLando This issue was already fixed in latest 5.3.0? I tried with this version and it's failing. What versions of In latest version I added
Then when running the package I get
|
@rightaway Did you have any luck using pkg with sharp? I'm in the same situation... TIA |
@avpavp No luck. @robertsLando says it should work in the latest version but that's definitely not true. |
@rightaway Same experience here. No success with [email protected] (current version at time of writing) By using bits and pieces of @webdevog repo mentioned above, I seem to be making progress using [email protected]. My exe runs now, I just need to make sure everything is actually working... |
I see the same issue with [email protected] it throws the as shown below, code runs fine from node directly:
|
Looking at the sharp repo, in
The problem is that when importing |
@leadwolf I think you detected the real problem. I also tried to move sharp into the same directory with no luck, I also tried adding both folders to assets:
and the entire folder:
Did you find a workaround to this? @jesec @erossignon suggestions? This should be handled here: https://github.com/vercel/pkg/blob/main/prelude/bootstrap.js#L2035 but for some reason it's not working as expected |
IMO what we should do is to copy the entire module folder instead of just the module, I will see if I can make a PR |
For anyone interested in this a fix is available with #1321 |
When a `.node` file is required `pkg` copies it in a temporary folder like `/tmp/<hash>/addon.node` the problem is that for modules like `sharp` this isn't working as them are statically linked to other `.so` files using relative paths. This pr will copy the entire module folder inside the temporary directory, this allows also to remove the `tryImport` function as there is no reason to try again if that fails. I tested this on my end and it's working I also grouped all hash folders inside `pkg` folder to be sure that the copy will run when users will update pkg, otherwise the copy may not be done when the folder already exists but was created with a previous pkg version Fixes #1075
see vercel/pkg#1319, this makes the package fail because of vercel/pkg#1075 rather than fail in the same way because of the missinng dependency
@robertsLando Is it still working for you? An error is happening again #1692. |
It's working for me. Try using
|
@robertsLando I tried again and it's actually working even without
|
Since updating to version 4.5.0 it is not possible to bundle sharp anymore. Previously this was fairly simple, by just copying the
node_modules/sharp
folder next to the build output. But this doesn't work anymore giving the following error:I'm fairly convinced this has to do with #837 or #1066 or maybe a combination of both.
Looking at the below error message running in CI, it seems that prebuild-install is not correctly installed or referenced, this has been added in #1066:
I'm mentioning both @david-mohr as well as @geekuillaume hoping they have a suggestion on how to fix this.
On a sidenote; this seems to me a breaking change so 5.0.0 would have been a more logical version for this release I suppose.
The text was updated successfully, but these errors were encountered: