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

[enh] Add the arch argument to ynh_install_nodejs #660

Merged
merged 4 commits into from Feb 24, 2019

Conversation

Projects
None yet
4 participants
@yalh76
Copy link
Contributor

commented Feb 22, 2019

The problem

When installing nodejs on arm64 and arch64, N - Node.js version management handle the architecture as AMD64 and the nodejs installation failed

Solution

Use the N - Node.js version management argument --arch to specify an architecture

PR Status

Done

How to test

There is a package with the functionality setuped: yunohost app install https://github.com/YunoHost-Apps/wikijs_ynh/tree/arm --debug

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :

@yalh76 yalh76 referenced this pull request Feb 22, 2019

Closed

nodejs on aarch64 #1311

@alexAubin

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Uh technically this looks good, but shouldn't we try to detect the arch and feed it to the node command, instead of leaving this job to the app packager ?

@yalh76

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Uh technically this looks good, but shouldn't we try to detect the arch and feed it to the node command, instead of leaving this job to the app packager ?

Well, in N, there is already an arch detection: https://github.com/tj/n/blob/master/bin/n#L411, so we don't need to implement one in ynh_install_nodejs.

The only issue is: for now, N arch detection doesn't work for aarch64/arm64 and N maintainer doesn't seem to want to implement it even if there is already a PR to solve it

@alexAubin

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Hmokay, but what I mean is why don't we add, in this helper, something like

if [[ $uname =~ aarch64 ]] then
    arch="arm64"
fi

and feed that to the node command ? I mean, otherwise the app packager will have to think about doing this him/herself, for each apps, and know how to do this and that doesn't look quite obvious ... ? Or am I missing something x_X

@yalh76

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Hmokay, but what I mean is why don't we add, in this helper, something like

if [[ $uname =~ aarch64 ]] then
    arch="arm64"
fi

and feed that to the node command ? I mean, otherwise the app packager will have to think about doing this him/herself, for each apps, and know how to do this and that doesn't look quite obvious ... ? Or am I missing something x_X

Good point, instead of adding an argument to ynh_install_nodejs, let him do the job... I will change the PR

remove arguments
and let ynh_install_nodejs manage the issue
@alexAubin
Copy link
Member

left a comment

LGTM

@alexAubin alexAubin added this to the 3.5.x milestone Feb 23, 2019

@alexAubin alexAubin changed the title add the arch argument to ynh_install_nodejs [enh] Add the arch argument to ynh_install_nodejs Feb 24, 2019

@JimboJoe
Copy link
Member

left a comment

LGTM 👍

@alexAubin alexAubin merged commit fa40232 into YunoHost:stretch-unstable Feb 24, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yalh76 yalh76 deleted the yalh76:patch-1 branch Mar 14, 2019

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