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

Galata fails to install due to node-canvas #12726

Closed
legendb317 opened this issue Jun 23, 2022 · 3 comments · Fixed by #12785
Closed

Galata fails to install due to node-canvas #12726

legendb317 opened this issue Jun 23, 2022 · 3 comments · Fixed by #12785
Assignees
Labels
bug dependencies Pull requests that update a dependency file

Comments

@legendb317
Copy link
Contributor

Description

When using a VPN, the installation of Galata fails due to the canvas dependency failing to install. The install of canvas fails because canvas tries to download from a link during the install process (It seems reasonable that a VPN would block this kind of thing). We have created an issue with canvas which has not had a response since it was opened a few months ago.

Without canvas making any changes, this seems like room to improve Galata to avoid situations like this. Canvas is only used in the benchmarkReporter.ts which uses playwright to benchmark performance in CI. Given that "Galata is a set of helpers and fixtures for JupyterLab UI Testing," it does not seem like benchmark performance test should be shipped with Galata. This will also guarantee that what is being shipped with Galata is as light weight as possible for the people using it.

Another, but less ideal, approach would be to add canvas as an optional dependency in the package.json, so that the install of canvas can fail silently. However, I would want to think that shipping Galata without the benchmarkReporter.ts would be better.

Thanks all for the help with this :)

@legendb317 legendb317 added the bug label Jun 23, 2022
@jupyterlab-probot jupyterlab-probot bot added the status:Needs Triage Applied to new issues that need triage label Jun 23, 2022
@welcome
Copy link

welcome bot commented Jun 23, 2022

Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please try to follow the issue template as it helps other other community members to contribute more effectively.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also an intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@trungleduc
Copy link
Member

@fcollonval might have insights about this issue.

@fcollonval
Copy link
Member

canvas was added without much thoughts to Galata because it was already a dev dependency of JupyterLab:

=> Found "canvas@2.9.1"
info Reasons this module exists
   - "_project_#@jupyterlab#csvviewer" depends on it
   - Hoisted from "_project_#@jupyterlab#csvviewer#canvas"
   - Hoisted from "_project_#@jupyterlab#debugger#canvas"
   - Hoisted from "_project_#@jupyterlab#terminal#canvas"
   - Hoisted from "_project_#@jupyterlab#galata#canvas"

Another, but less ideal, approach would be to add canvas as an optional dependency in the package.json, so that the install of canvas can fail silently. However, I would want to think that shipping Galata without the benchmarkReporter.ts would be better.

I'm more for the opposite (aka setting canvas as optional dep). The main reason is that we have already lots of packages and I don't want to create yet another one for the benchmark helpers. We can then simply switch the benchmark reporter to load asynchronously canvas and not generate a graph if it is not there.

I'll be happy to review a PR.

Note: Using optional dep will also allow to backport the changes and not wait for 4.0 (that will happen if we extract benchmark code from galata).

@JasonWeill JasonWeill added dependencies Pull requests that update a dependency file and removed status:Needs Triage Applied to new issues that need triage labels Jul 7, 2022
@fcollonval fcollonval self-assigned this Jul 8, 2022
fcollonval added a commit to fcollonval/jupyterlab that referenced this issue Jul 8, 2022
fcollonval added a commit that referenced this issue Jul 18, 2022
* Use SVG renderer in Vega to drop canvas dependency
Fixes #12726

* Fix tests

* Update Playwright Snapshots

* Rename variables

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
fcollonval added a commit to jtpio/jupyterlab that referenced this issue Jul 19, 2022
* Use SVG renderer in Vega to drop canvas dependency
Fixes jupyterlab#12726

* Fix tests

* Update Playwright Snapshots

* Rename variables

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
(cherry picked from commit 2bd9933)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants