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

Select a node-gyp versions with python3 support #4407

Closed
wants to merge 1 commit into from
Closed

Select a node-gyp versions with python3 support #4407

wants to merge 1 commit into from

Conversation

cryptomilk
Copy link
Contributor

First time contributor checklist:

Contributor checklist:

Description

Select a node-gyp versions with python3 support

See also
sass/node-sass#2841
sass/node-sass#2716

@scottnonnenberg-signal
Copy link
Contributor

Why have different resolutions in both places? Shouldn't both node-gyp resolutions just be 6.1.0?

@cryptomilk
Copy link
Contributor Author

Ups, fixed.

@scottnonnenberg-signal
Copy link
Contributor

Also, please eliminate the ^ - in our package.json we map to specific versions.

@kpcyrd
Copy link
Contributor

kpcyrd commented Nov 4, 2020

Is there a way to help getting this merged? :)

@scottnonnenberg-signal
Copy link
Contributor

It has a conflict now, but there is something else missing - what's the benefit provided by taking on the risk of these changes?

@kpcyrd
Copy link
Contributor

kpcyrd commented Nov 19, 2020

As an alternative approach, node-sass very recently released its 5.0.0 which doesn't depend on python2 anymore, so modifying dependency resolution is not necessary anymore. :)

@WhyNotHugo WhyNotHugo mentioned this pull request Jan 11, 2021
1 task
package.json Outdated Show resolved Hide resolved
@hiqua
Copy link
Contributor

hiqua commented Feb 15, 2021

It has a conflict now, but there is something else missing - what's the benefit provided by taking on the risk of these changes?

@scottnonnenberg-signal Is this question answered by the issue linked here? #4783

To elaborate on these: plenty of distributions are in the process of not shipping python2 by default. The next Debian stable (due in the next few months) won't.

node-gyp just dropped support for python2 for its new versions: nodejs/node-gyp#2300

In short, it's not like this change can be avoided. It will have to be done eventually anyway. It might as well be done now that it's still possible to come back on it if something fails unexpectedly and that there's no big rush. It's also an unnecessary barrier to contributions, since people will be increasingly reluctant to install python2 just to contribute minor changes, as @WhyNotHugo has mentioned (e.g. I'm currently trying to build signal-desktop on Debian and I cannot for sure exclude python2/ python3 conflicts for my failing to do so).

Copy link
Contributor

@WhyNotHugo WhyNotHugo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lockfile needs updating too. Otherwise, CI just uses the previously locked version: v5.0.3.

@WhyNotHugo
Copy link
Contributor

#4784 works and is ready for review. I'd really appreciate if a dev can look at this soon.

See also
sass/node-sass#2841
sass/node-sass#2716

Signed-off-by: Andreas Schneider <asn@cryptomilk.org>
@cryptomilk
Copy link
Contributor Author

Closing in favor of #4407

@cryptomilk cryptomilk closed this Apr 9, 2021
@WhyNotHugo
Copy link
Contributor

I think you meant closing in favour of #4784 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants