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

Remove static_library for arm architectures to build keytar.node also… #92

Merged
merged 1 commit into from May 24, 2019

Conversation

@surferandi
Copy link
Contributor

commented Jan 5, 2018

… on raspberry pi, see #91

@NDNELSON

This comment was marked as off-topic.

Copy link

commented Jan 5, 2018

Excuse I am New in this. How can I remove ir? Or where is it?

@50Wliu 50Wliu added the needs-testing label Jan 5, 2018

@tzarebczan

This comment has been minimized.

Copy link

commented Jul 24, 2018

Hi, can we get this reviewed? We are also running into the same problem on the pi.

@kodxana

This comment has been minimized.

Copy link

commented Jul 24, 2018

This pull fix ARM build hope you merge it soon :)

@shiftkey

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2018

@tzarebczan @surferandi @kodxana it's not clear to me how this fixes the underlying issue. #79 had a decent writeup about the issue they were seeing with linking to shared libraries, and I'd love similar context for this change because I don't have access to ARM hardware myself.

Are you able to provide the npm install output before this PR (showing the underlying issue) and with this change (confirming it's resolved)?

@kodxana

This comment has been minimized.

Copy link

commented Jul 24, 2018

@shiftkey ['target_arch=="arm"', { 'type': 'static_library' }]
Raspberry Pi is armhf or armv7l. So arm is not correct architecture for Rpi so it throw error when building module.

@shiftkey

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2018

@kodxana so this should be target_arch=="arm64" then, so it doesn't get applied for other architectures?

cc @stevedesmond-ca would love your input on this

@kodxana

This comment has been minimized.

Copy link

commented Jul 24, 2018

@shiftkey target_arch=="armhf" this is architecture used by most of the Pi

@stevedesmond-ca

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2018

My suggestion would be to change the arm override to specifically call out each of armel, armhf, and arm64. This seems to be the way things are headed now that ARM is becoming a more common target.

ARM processors basically come in 3 flavors days:
v6: 32-bit, no floating point hardware (armel)
v7: 32-bit, with floating-point hardware (armhf)
v8: 64-bit, with floating point hardware (arm64)

Raspbian (the de-facto Raspberry Pi flavor of Linux) is 32-bit only last I checked. While other distros exist that target 64-bit hardware, they're definitely not the majority. On the Chromebook side of things, it's currently about a 50/50 split between 32- and 64-bit CPUs, for those that use ARM. Pretty much all new smartphone hardware being made today is 64-bit, but there's still a lot of 32-bit out there.

@shiftkey

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

Apologies for the delay in getting back to this. This has been really hard for me to test due to lack of insight into the ARM ecosystem and what we should be doing here, but I'm going to take this in and see if it makes these use cases better.

@shiftkey shiftkey merged commit 135573d into atom:master May 24, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shiftkey

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

This change is available in version v4.9.0 on NPM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.