-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
"Cannot find module 'sharp'" on Windows only after upgrading to 26.0.11 #8970
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
Comments
Hiya! Can you share a minimum reproducible repo that I can test locally with and/or create a unit test around? Would like to assist but need more information first. |
Hey @mmaietta thanks for your reply. I did find the problem - in
Simply removing this is enough of a fix for us and probably only adds a tiny bit to the package size, but I do wonder why that would now be breaking in 26.x (and seemingly only on Windows.) Thanks again! |
Actually, I must have overlooked something because that's definitely not resolving the issue now... |
Hm... I really thought I had this working but every attempt now results in this same issue. I'm not sure if I changed something else or if what I thought fixed it never really did. I'll update soon. |
@mmaietta I did notice that there's nothing under |
Yeahhhh, definitely need a minimum repro repo for this unfortunately. The node collector logic was completely migrated to TS from Golang to support pnpm amongst other things, and a migration to |
Thanks as always for the great work and for your help @mmaietta! I put together a repro here: https://github.com/Nantris/electron-builder-repro-8970 It requires Yarn v4 (which I think should be used automatically as long as you have corepack enabled?) Repro steps:
This isn't an exact copy of our config and may not be 100% correct in every regard, but it should at least output the |
Well this is odd indeed. Your |
Thanks as always for all you do @mmaietta! I was wondering if it might be related to Yarn 4 myself. We upgraded from 25.1.8 and I believe I verified this affects 26.0.0 specifically but I'm not certain now. It's super strange to me that while Windows fails, Linux builds remain unaffected. I haven't had a chance to test macOS. Maybe there's some sort of path issue that makes this only work on POSIX? |
I confirmed macOS is unaffected. So only Windows has the problem. I also tried setting |
I am experiencing this issue too, but with a different dependency. this works with 26.0.2 (before the node_module collector change), but not with 26.0.11. I am using yarn 4.7.0 If I look inside the app.asar, there is no node_modules folder at all, we have 10+ direct dependencies that should be included there. I should add a disclaimer that we are using a lightly patched app-builder-lib, but that should only be including more node_modules than usual loose inside the resources directory. The snippet of our config that I expect could affect this is:
|
@beyondkmp can you please take a look at this when you have a chance? Seems there's an issue specifically on Windows, and it may be specific to yarn 4. I'm trying to write a unit test from my side, but once corepack is enabled in the docker container to support a yarn4-scoped test, subsequent runs of other unit tests break due to corepack now being enabled. |
@mmaietta that's very strange! Because our macOS build worked unless I'm crazy. Edit: Reconfirming macOS built properly with |
Hmmm, I'm running off latest I was able to resolve the dependency collection by changing the
Trying to figure out how to get around this issue now, as at least the dependency tree is being returned correctly. Suggestions welcome as I'm still pretty sick and my mind is an absolute ball of fuzz right now |
I'm getting similar problem on 26.0.11 - multiworkspace project, unpacking asar shows there are no node_modules there at all. |
@stq, is that also a yarn 4 project? |
No, npm9/node20. Downgrading to 26.0.2 fixed issue. |
@stq would you mind putting together a minimum reproducible repo since yours isn't yarn 4? I'd like to create a dedicated unit test for this multi-workspace project as we already have some two-package.json app fixtures, but it seems like we're missing a use case. |
@Nantris can you try this patch-package? It works all of a sudden for me on macos target, still need to setup a test env in a windows VM though
|
@mmaietta I'm sorry to report it didn't resolve the issue. But macOS always worked for us, so maybe there are two issues here? It's pretty confusing. |
@mmaietta you're testing on the master branch, right? Unfortunately I can't get that working here, apparently due to this issue, so I guess we may be testing slightly different setups. That image is from macOS, right? |
This will include all dependencies from projectDir, while the dependencies in appDir should only be a subset of projectDir's dependencies and don't need to be entirely included. |
Should be the same setup. That was on a default |
@mmaietta this is my bad, but actually even more confusing. Originally I tested against our project and not against the repro. The patch works on the repro :) Does not work on our project :( Edit 1: Investigating further. If I come across anything I'll update. |
Oh lordy, this is becoming a conundrum n' a half. I was going to suggest this as an easy fix. Checks the
But since it doesn't work for your original/source project, I'm not sure where to go from here either. Might need @stq to get back to us on a minimum repro project so I can do a deeper dive since this doesn't seem isolated to yarn 4. Might be due to multi-workspace setups, which is my hunch, but I need a local setup to repro and write a unit test for. Otherwise I'm just stabbing in the dark to find a needle in a haystack 😅 I think it may be useful to add a |
The repro I provided does indeed fail for me on macOS, whereas my project does not.
Seems very possible, since our project is indeed a Yarn workspace, but all the native deps are at the project root so one wouldn't think it would be the cause, but it's the most promising theory I'd say. The only other differences I can find are:
I noticed some debug statements in For now I guess we're going to stick with 26.0.2 and pin hopes on an independent repro hopefully pointing to whatever this other issue is. |
Set env var |
@Nantris When you have time, Could you provide your dependencies and project results so we can try to reproduce the issue? |
@beyondkmp it's a rather sprawling monorepo so while I can provide a list of native dependencies, providing a list of all the dependencies and how they're specified isn't likely to be feasible. Could you clarify what you mean by project results? I guess specifically the contents of |
@beyondkmp I wonder if #8571 is likely to be related to this? |
It is indeed related to this. However, we are currently unable to reproduce your issue, so we need your help in providing the project structure and |
I appreciate how helpful you guys are! Unfortunately I don't know what we can realistically share. If not for this being a monorepo it would be easier to suggest what possibly to try. Although all of our native dependencies live in the root Besides providing a repro, I wonder if you might have any advice about where I could throw some logging lines in the code changed in #8571. Maybe I can figure out more precisely where it fails and report back directly with that. |
@Nantris are you in the ElectronJS Discord group by chance? You can reach out to me via there (I'm grouped under "maintainer" in the |
Thank you both! I'll loop back to check if those fix them once they're released. Otherwise I'll find you guys on the |
Released v26.0.13 |
Looks good! Problem is resolved! Thank you so much! |
Quite confusingly, this is broken again. I wonder if maybe a dependency of Unfortunately I don't have a working |
I really can't find anything different. Here's the story:
I tried messing with the dependencies of The only thing I can think is that my first test of |
Experiencing something similiar after upgrading from 25.1.8 (works), to 26.0.12 (doesn't) on mac (haven't tested windows yet). We use yarn v3 in a monorepo setup with dual pkg.json and after upgrading no node_module folder is packaged into the asar. (and consequently asarUnpack has nothing to unpack). Relevant settings (simplified):
I can confirm these directories to exist:
Now after upgrading to 26.0.13 I can confirm the After running the packaged app I get:
I can see that Now if I roll back to 25.1.8 again, things work just fine. I saw that import preGyp from '@mapbox/node-pre-gyp'; although its only listed as Does that confuse the I did try to add |
@tom2strobl It's mainly related to the structure of your monorepo. Can you provide a minimal reproducible demo similar to your monorepo's structure? You can refer to the demos provided in these two issues(#9000 & #9011) |
@beyondkmp I see, thanks for getting back to me. Here's a minimal repro: https://github.com/tom2strobl/electron-builder-monorepo-repro Let me know if I can help any further! Appreciating you guys a lot |
Fwiw I don't think it's anything specific to Thanks for putting together a repro! |
@tom2strobl root cause The Due to the limitations of npm list in this configuration, the only solution I can think of at present is to modify your configuration to solve this problem. There are two solutions:
Additionally, the postinstall script |
Perhaps not an ideal solution, but I'm pretty sure that we work around that same issue (?) by just putting native dependencies in the root monorepo dependencies, rather than in a subpackage. |
@beyondkmp thanks a lot for the swift reply!
Thanks for the explanation! I love it when the answer is not just a black magic box
Sorry if that's a dumb question but whats the point of a dual package.json setup if they have identical dependencies? Just the Wasn't the point that only native dependencies go into the the "production" pkg.json so we know which ones to rebuild? Hoisting was necessary to make sure they are physically in a different location. Being able to hoist was pratically the only reason we had to use yarn because most monorepo setups fail if you need multiple Or wait... are you essentially implying all of this is not necessary anymore? Could you elaborate how you know which We currently need to |
It should now be possible to automatically rebuild native modules since it has been switched to Indeed, the You can try Solution 2: Remove the following from the desktop-release package.json
delete that configuration and check if the app can be packaged and run correctly? |
@beyondkmp
Our // file is truncated for brevity
import { rebuild } from '@electron/rebuild'
const result = await rebuild({
// desktop-release
buildPath: join(import.meta.dirname, '..', '..', '..', 'desktop-release', 'app'),
// actual monorepo root
projectRootPath: join(import.meta.dirname, '..', '..', '..')
}) I got it running in dev when electron-builder is out of the picture. Now when I try to package there are a couple of weirdnesses: First of all, whenever I run
Removing the When trying to package it also errors:
seeing the
I tried running it from the repo root but that (nicely) errors with I reverted everything to how it was before and used |
Your mono repo is a bit too complex. So, let's just remove this configuration. At least in your repro demo, removing this configuration allowed the build to succeed.
|
@beyondkmp If I remove the hoistingLimits in the repro demo I
Internal Error: buildermono@workspace:.: This package doesn't seem to be present in your lockfile; run "yarn install" to update the lockfile So it appears to me that some change after 25.1.8 just generally rendered yarn workspaces incompatible, even though the docs actually recommend using yarn. |
@tom2strobl |
Package manager can be defined in the package.json, I think under the engines key? I know it's a bit of an escape hatch, but having people manually specify it in these edge cases doesn't seem too crazy as a workaround. |
After upgrading to
26.0.11
it seems Sharp isn't being properly bundled or run. None of our config changed except changingwin.publisherName
towin.signToolOptions.publisherName
.I'm not sure how to go further in investigating this. Any advice appreciated!
The text was updated successfully, but these errors were encountered: