Skip to content
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

Update 'Getting Started' to include non-Node libs #12018

Closed
wants to merge 1 commit into from

Conversation

DylanLacey
Copy link

List details of libraries needed on MacOS when installing from scratch.

@Snuffleupagus
Copy link
Collaborator

@timvandermeij
Copy link
Contributor

timvandermeij commented Jun 23, 2020

Moreover, why are these necessary? PDF.js doesn't rely on them, and we have contributors working on Mac as well and never heard about these being required. I can only see node-gyp being used by the canvas dev dependency which does seem to have some problems upstream providing pre-built packages, see https://github.com/node-gfx/node-canvas-prebuilt/issues, but I'd rather not ask people to install such dependencies globally. If that remains a problem, we might see if we can move the canvas dependency out of package.json since it appears to be only relevant for the PDF to PNG example. We could solve that in the same way as we did with some Webpack-specific packages.

@DylanLacey
Copy link
Author

DylanLacey commented Jun 24, 2020

Do you mean why are these libraries necessary, or why are these instructions necessary?

RE Instructions, I found that, on an almost new Mac system (MacOS 10.15.5), the instructions in the README did not work. I couldn't npm install successfully until I'd installed canvas & friends manually via brew. My suspicion would be that other contributors already have these deps installed or, as you said, packages were pre-built at the time they followed the README.
I'm happy to document this elsewhere such as the FAQ or a new Wiki page instead if you'd prefer that. There's Setting up PDF.js Dev on Windows ; I could add one for MacOS as well?

RE libraries, I can't comment :P I was only focusing on making the README more useful to new contributors.

Added details on installing pango, cairo and pixman through brew, to avoid node-gyp installation problems.
@tamuratak
Copy link
Contributor

tamuratak commented Jun 24, 2020

Related to Automattic/node-canvas/issues/1587. Adding the package request to package.json solves the issue. Since the instruction is deleted from Automattic/node-canvas/issues/1511, I am not sure it is an appropriate one at present.

@timvandermeij
Copy link
Contributor

OK, so the only reason we need these extra steps here that were not necessary before is because upstream does not provide valid prebuilt canvas packages anymore, at least not without adding a dependency to package.json that we don't even use, or we need to install extra packages to compile canvas.

I think a better solution here would be to do neither or the two, and instead do something like https://github.com/mozilla/pdf.js/pull/11964/files where we mention in the README file of the example that a prerequisite is installing canvas. This way a regular clone of PDF.js will not require this dependency at all anymore. We only need to check if canvas is indeed only used for the example.

@Snuffleupagus
Copy link
Collaborator

We only need to check if canvas is indeed only used for the example.

The canvas package is primarily used for the unit-tests, when running on Node.js/Travis; please see

class NodeCanvasFactory {
create(width, height) {
assert(width > 0 && height > 0, "Invalid canvas size");
const Canvas = require("canvas");

@timvandermeij
Copy link
Contributor

Hm, I'm wondering if adding the request package temporarily (until upstream issues are fixed) is the least bad solution here. For me it's better than asking people to install software globally on their workstation. Opinions?

@tamuratak
Copy link
Contributor

Another solution is executing the following explicitly:

npm install --save-dev canvas

Then, prebuilt binaries and required libraries are installed in node_modules/canvas/build/Release without errors. I do not know why this works well. After the installation, npm install causes no error.

One thing making things complicated is that the package request is deprecated. See https://github.com/request/request#deprecated

The following is my configuration.

OS: macOS Highe Sierra 10.13.6
npm: 6.14.5
node: v12.16.3

DylanLacey pushed a commit to DylanLacey/pdf.js that referenced this pull request Jun 25, 2020
Versions of `needle` prior to `2.5.0` cannot cope with redirects (as documented: tomas/needle#312).

This prevents prebuilt `canvas` binaries from being downloaded on MacOS,
requiring the global install of its dependencies.

Updating Needle restores it to functionality, addressing this.  It also avoids
the need to add `request` to `package.json`; it also obsoletes mozilla#12018.
@DylanLacey
Copy link
Author

I was surprised that the needle issue appeared to be addressed (according to the bug report) and I wanted to try resolving this with request... but since it's deprecated, I thought I'd take a closer look or by re-open the needle bug. So, I uninstalled the libraries using brew, blew away my node_modules and tried installing again and.... it worked?

Which was a bit odd, until I compared the checkout I made directly from the project 2 days ago (instead of my fork), and found that needle had installed at 2.3.something instead of 2.5.0. No idea why, since both were new checkouts and 2.5.0 was released last month, but um.... OK then 🤷 .

"But Dylan, get to the point!" you reasonably cry: #12024 forces needle to 2.5.0 or higher and this issue is obsolete; No need to replace dependencies, bundle packages or update READMEs.

@DylanLacey DylanLacey closed this Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants