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] Upgrade n version (also for compatibility with arm64) #753

Open
wants to merge 1 commit into
base: stretch-unstable
from

Conversation

@yalh76
Copy link
Contributor

commented Jul 10, 2019

The problem

  1. When installing node js using ynh_install_nodejs on aarch64, it's the x86-64 version of nodejs that is installed
  2. N - Node.js version management included in YunoHost is version 2.1.7, one year old

Solution

Updating N - Node.js version management version
since N version 3.0.0 aarch64 and arm64 are detected

PR Status

Done

How to test

Have and aarch64 or arm64 YunoHost, install a package that request nodejs like wekan: yunohost app install https://github.com/yalh76/wekan_ynh/tree/2.98 --debug

After the installation, just test someting like: /opt/node_n/bin/node --version

Validation

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2019

Bump :)

@Psycojoker

This comment has been minimized.

Copy link
Member

commented Jul 28, 2019

LGTM by principle but I'm not really in the app group '-'

@decentral1se
Copy link
Contributor

left a comment

Cool.

@alexAubin

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Looks okay to me on the principle, but what about existing instance ? Is a migration required somehow or will it happen automagically when upgrading apps or something ?

@yalh76

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Looks okay to me on the principle, but what about existing instance ? Is a migration required somehow or will it happen automagically when upgrading apps or something ?

Maybe @maniackcrudelis can have a better response than mine as he seems to better know that helper.

After reading the code of this helper, n won't be upgraded automagically but as far as I understand the helper, a migration to the new version is not required

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

Haven't been noticed for this PR by github...
Maybe I should stop working on nuclear weapons 🤔 ...

N won't be upgraded, this function is called only if n isn't already install.

# If n is not previously setup, install it
if ! test $(n --version > /dev/null 2>&1)
then
ynh_install_n
fi

@alexAubin alexAubin changed the title upgrade n version [enh] Upgrade n version (also for compatibility with arm64) Aug 5, 2019

@alexAubin alexAubin added this to the 3.7.x milestone Aug 5, 2019

@alexAubin

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

N won't be upgraded, this function is called only if n isn't already install.

Uuuuh wokay, so shall we edit this helper to check that the version installed is the one expected by yunohost and upgrade it otherwise ?

(Or we can also add a migration using snake)

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

Since the condition is testing the version, and that we now have an upgrade of this version.
Guess we could adapt the helper to update n in case the version is different.

@yalh76

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

After making a test, it seems that N is automagically updated....

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