Skip to content

improve compatibility with newer node versions #1

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

Closed
wants to merge 3 commits into from

Conversation

prototype99
Copy link

this is done by bumping the canvas package version. i have put the commit underneath pdf-reader's version to make sure rebasing is already done. Zotero built with this version of pdf.js had no immediately obvious issues, including opening and reading through a few pdfs.

@mrtcode
Copy link
Member

mrtcode commented Mar 21, 2022

So you just want the canvas library to be updated? What exactly is the problem?

@AbeJellinek
Copy link
Member

AbeJellinek commented Mar 22, 2022

(Not @prototype99, but) compiling our pdf.js version with Node 17 [on ARM In my case] always fails and can only be fixed by tinkering with the package.json in a way that always seems to be different every time it breaks. It's very frustrating! The error message indicates a failure in fetching prebuilt canvas binaries for Node 17 on ARM, so I'm not sure if it's one or both of those that's causing the issue.

@mrtcode
Copy link
Member

mrtcode commented Mar 22, 2022

@AbeJellinek On Node 17 [on ARM as well] it fails for me to install puppeteer "The chromium binary is not available for arm64.". I haven't investigated further.

Later this year we plan to catchup with the latest PDF.js version.

But if you are saying that just updating canvas version makes everyone's life easier, we can do it now, of course.

@AbeJellinek
Copy link
Member

@mrtcode: You can fix that by adding

export PUPPETEER_SKIP_CHROMIUM_DOWNLOAD=true
export PUPPETEER_EXECUTABLE_PATH="/Applications/Chromium.app/Contents/MacOS/Chromium"

to your shell config, assuming you have Chromium installed /Applications.

@prototype99
Copy link
Author

Essentially: the combination of the package.json and the package-lock.json pulls in "node-pre-gyp": "^0.11.0" which resolves to '[email protected]'. I tried just updating that package, however it was still pulled in at version '0.11.0' which would cause failures. The only package depending on that was canvas, so that seemed to be the problem. I assume it is a node versioning issue, so I looked for a version of 'canvas' with newer node version compatibility. '[email protected]' notably both updated 'node-pre-gyp' and mentioned adding node 16 to the CI so it seemed like a good candidate, and after that one change ( npm i [email protected] ), npm i now does not fail, which it did before. much like @AbeJellinek I was using node 17, not arm though, so I can't speak from experience on any arm issues, i was using x86_64.

@mrtcode
Copy link
Member

mrtcode commented Mar 24, 2022

Thanks @AbeJellinek. That fixes the Chromium problem.

Unfortunately, at least for me, canvas 2.8.0 still can be installed, so it seems this isn't the only problem.

npm ERR! node-pre-gyp http GET https://github.com/Automattic/node-canvas/releases/download/v2.8.0/canvas-v2.8.0-node-v102-darwin-unknown-arm64.tar.gz
npm ERR! node-pre-gyp ERR! install response status 404 Not Found on https://github.com/Automattic/node-canvas/releases/download/v2.8.0/canvas-v2.8.0-node-v102-darwin-unknown-arm64.tar.gz 
npm ERR! node-pre-gyp WARN Pre-built binaries not installable for [email protected] and [email protected] (node-v102 ABI, unknown) (falling back to source compile with node-gyp) 
npm ERR! node-pre-gyp WARN Hit error response status 404 Not Found on https://github.com/Automattic/node-canvas/releases/download/v2.8.0/canvas-v2.8.0-node-v102-darwin-unknown-arm64.tar.gz 

@prototype99
Copy link
Author

hmm isn't darwin and arm64 the M1 macs? The location it tries to fetch from only has x64 darwin (intel macs?) prebuilt binaries. Does it work for you on an x86_64 device?

Automattic/node-canvas#1662 may be the issue for arm, it appears that arm is a bit more involved, you have to follow this process: https://github.com/Automattic/node-canvas/wiki#installation-guides

from what i can tell, m1 macs may additionally only work with version 2.9.0 onwards because it merges an m1 feature branch. hopefully this helps

@mrtcode
Copy link
Member

mrtcode commented Mar 25, 2022

2.9.0 doesn't help. I can of course remove canvas module completely because it's not necessary for our PDF.js build.
But with gulp generic I still get:

Error: error:0308010C:digital envelope routines::unsupported
    at new Hash (node:internal/crypto/hash:67:19)
    at Object.createHash (node:crypto:130:10)
    at BulkUpdateDecorator.hashFactory (************/pdf.js/node_modules/webpack/lib/util/createHash.js:144:18)

So, again, we can update canvas module or even remove it, but that doesn't solve the problem completely if you still can't build on arm64. I guess there are more packages that have to be updated.

In around half a year we will update to the latest PDF.js version, and hopefully all the problems with arm64 will be fixed.

@prototype99
Copy link
Author

Even when canvas is manually built as described in the respective installation guide for your OS? it seemed to be suggested that canvas can then be installed on arm64 without issue (i've been assuming that's the only arm we're talking about)

regardless, just to make sure I understand: either way as things are, arm support is broken for now, however removing canvas in general is a preferred solution, when compared to updating the version of canvas.

It appears that the issue experienced with webpack is also to do with node 17, i'm not sure why i didn't experience it, however i wil explore solutions

@prototype99
Copy link
Author

prototype99 commented Mar 25, 2022

the issue you experienced should be fixed by npm i [email protected], i did not notice any issues with the webpack 5.61.0 build. once i know for sure if we're keeping or removing canvas i'll update the pull request to reflect that

@prototype99
Copy link
Author

I see now, the fact it was in the package.json was from a file conflict that resolved incorrectly. This new version rebases in and cherrypicks the original commit. I have rebased the entirety of the zotero pdf.js fork, I will explain further on zotero/zotero#2183 once I'm happy that all the commits are correct

@mrtcode
Copy link
Member

mrtcode commented Oct 3, 2022

canvas and puppeteer packages removed in 7ea75b5.

@mrtcode mrtcode closed this Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants