Use a node-gyp version which supports Python3 #4784
Conversation
|
Linting is failing, not entirely sure why: Have to jump to work now, but will try to debug this futher at the end of the day. |
Did you figure out why lint-deps is failing and how to fix it? I've been trying to figure out what exactly this command does but I'm coming up blank. Maybe a Signal team member could shed some light here? |
|
It runs I think we're going to need the input of one of the devs here. |
|
Note that, you can create a development environment with this branch, and the do you work based off |
I agree, I can't wrap my head around that massive exceptions.json file. I opened an issue #4952 to hopefully consolidate communication and help with SEO. Seems a few other pull requests ran into the same problem without ever getting it resolved. |
|
@scottnonnenberg-signal: it seems you did some previous work on this area, do you have any insight? |
|
@WhyNotHugo could you rebase your changes? |
|
I did a manual rebase, but it won't install for me. I tried re-doing the work in this commit, but it does not work either.
Not entirely sure what changed in the |
|
@WhyNotHugo see #4995 (comment) You need to have git-lfs installed, and possibly run You might have to clean the yarn cache before you try again: |
|
Oh, nice, thanks! Slipped my mind to re-read CONTRIBUTING.md, and it's been added there as a dependency very clearly 🤦 Rebased properly this time! :) |
|
|
|
AFAICT it checks for bad patterns in the dependencies, just like the A solution could be to whitelist |
|
I've added it as an exception there. I guess whenever it's reviewed by a Signal developer we'll know if it's the right place. Not sure how to actually get some feedback here though. It's been over a month. |
|
Figured it out. There's some linting rules, and there a list of files that are exempt (I guess they've been determined safe). However, these exemptions are listed with their filename and line number. The upgrade in
|
WhyNotHugo
changed the title
|
@scottnonnenberg-signal Any feedback for this? |
|
FWIW I run into the other side of the coin of this issue: Ironically the whole point of PEP394, and removing I sent a PR for the above to journeyapps: https://github.com/journeyapps/node-sqlcipher/pull/80/files Update Also sent a PR to Signal's fork, in case upstream never gets to that: EvanHahn-Signal/node-sqlcipher#2 But in the meantime, if you run into this, you can hack your way around it by telling yarn to pull from my repo (see below). OBVIOUSLY I'm not proposing you change the upstream, but since this PR is the only one tending to python issues, this might be useful to anyone else losing hours into making sense of this. diff --git a/package.json b/package.json
index 6ace400a..dffd8eee 100644
--- a/package.json
+++ b/package.json
@@ -63,7 +63,7 @@
"fs-xattr": "0.3.0"
},
"dependencies": {
- "@journeyapps/sqlcipher": "https://github.com/EvanHahn-signal/node-sqlcipher.git#16916949f0c010f6e6d3d5869b10a0ab813eae75",
+ "@journeyapps/sqlcipher": "https://github.com/diegoe/node-sqlcipher.git#c30f38722dcc97a497835c234bdc913a38710160",
"@sindresorhus/is": "0.8.0",
"@types/pino": "6.3.6",
"@types/pino-multi-stream": "5.1.0",
diff --git a/yarn.lock b/yarn.lock
index 02670e08..0fd91e61 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -1336,9 +1336,9 @@
resolved "https://registry.yarnpkg.com/@icons/material/-/material-0.2.4.tgz#e90c9f71768b3736e76d7dd6783fc6c2afa88bc8"
integrity sha512-QPcGmICAPbGLGb6F/yNf/KzKqvFx8z5qx3D1yFqVAjoFmXK35EgyW+cJ57Te3CNsmzblwtzakLGFqHPqrfb4Tw==
-"@journeyapps/sqlcipher@https://github.com/EvanHahn-signal/node-sqlcipher.git#16916949f0c010f6e6d3d5869b10a0ab813eae75":
+"@journeyapps/sqlcipher@https://github.com/diegoe/node-sqlcipher.git#c30f38722dcc97a497835c234bdc913a38710160":
version "5.0.0"
- resolved "https://github.com/EvanHahn-signal/node-sqlcipher.git#16916949f0c010f6e6d3d5869b10a0ab813eae75"
+ resolved "https://github.com/diegoe/node-sqlcipher.git#c30f38722dcc97a497835c234bdc913a38710160"
dependencies:
node-addon-api "^3.0.0"
node-pre-gyp "^0.15.0"
|
|
Looks like this needs rebasing! |
|
There's other dependencies that don't build for me: I've wasted too many hours on this project, and maintainers clearly don't care about contributions. There's plenty of references to this PR by many contributors hitting this issue, yet, much like on many other PRs, maintainers just don't care. Even though this PR has been in a "ready" state for months. Apparently, expectation is that people to build and install ancient, unsupported software in order to develop on Signal (just to later ignore contributions anyway). |
|
I don't think it makes sense to rebase until the devs have approved this change at least in spirit. Once (if) they have, I'm willing to do the rebase. |
The currently pinned version of `node-gyp` requires Python 2. Python 2 has reached its EOL a long time ago and is unsupported by upstream. The requirement on Python 2 is also an unnecessary barrier for new developers to contribute to Signal. This changeset updates the required version of node-gyp to one compatible with Python 3. `CONTRIBUTING.md` remains unchanged, since this dependency was not previously mentioned anyway. Supersedes #4407 Fixes #4783
|
I've rebased, but can't fully tests locally. However, I also have this problem on |
Awesome, thanks! As long as it succeeds on GH it should be ok, otherwise I can help out. It might be about deleting the |
|
This is likely an issue with |
|
I'd cleaned via
|
|
Thanks! We'll plan to merge this after v5.0.0 is released to production. |
The currently pinned version of
node-gyprequires Python 2. Python 2has reached its EOL a long time ago and is unsupported by upstream.
The requirement on Python 2 is also an unnecessary barrier for new
developers to contribute to Signal.
This changeset updates the required version of node-gyp to one
compatible with Python 3.
CONTRIBUTING.mdremains unchanged, since this dependency was notpreviously mentioned anyway.
Supersedes #4407
Fixes #4783
First time contributor checklist:
Contributor checklist:
developmentbranchyarn readyrun passes successfully (more about tests here)