Improving go helper #2023

yalh76 · 2024-12-29T04:50:36Z

The problem

There was some issues and missing parts in the go helper

Solution

Use the Ruby helper as starting point to improve the go helper

PR Status

Ready

How to test

Put it in package like lxd and use it / I can do some test PR to show that it works

helpers/helpers.v1.d/go

Salamandar · 2025-01-05T22:31:55Z

helpers/helpers.v1.d/go

-        ynh_print_info --message="Downloading goenv..."
-        git init -q
-        git remote add origin https://github.com/syndbg/goenv.git
+    goenv="$(command -v goenv $goenv_install_dir/bin/goenv | grep "$goenv_install_dir/bin/goenv" | head -1)"


This command will fail if goenv is not present. You should something like that instead:

if command -v …

In that case

yunohost/helpers/helpers.v1.d/ruby

Line 135 in a96cb9b

rbenv="$(command -v rbenv $rbenv_install_dir/bin/rbenv | grep "$rbenv_install_dir/bin/rbenv" | head -1)"

and

yunohost/helpers/helpers.v2.1.d/ruby

Line 77 in a96cb9b

rbenv="$(command -v rbenv $RBENV_INSTALL_DIR/bin/rbenv | grep "$RBENV_INSTALL_DIR/bin/rbenv" | head -1)"

should not be ok

To me what's troublesome is : what is it doing in the first place x_X

It looks like this is meant to verify that $RBENV_INSTALL_DIR/bin/rbenv exists (and maybe is executable) ? Shouldn't we just use something like

rbenv="$RBENV_INSTALL_DIR/bin/rbenv" if [[ -x "$rbenv" ]] then ... else ... fi

(In fact that was the original code x_x)

alexAubin · 2025-01-10T22:28:35Z

helpers/helpers.v1.d/go

-    local installed_apps=$(yunohost app list --output-as json --quiet | jq -r .apps[].id)
+    local installed_apps=$(yunohost app list | grep -oP 'id: \K.*$')


Uuuuh not sure why switching to the new version ... the current one with json and jq seems much more clean and robust ? Is it that the ruby helpers still use an hacky grep ?

Yes ruby helper still using grep

yunohost/helpers/helpers.v2.1.d/ruby

Line 231 in f7ed895

local installed_apps=$(yunohost app list | grep -oP 'id: \K.*$')

and

yunohost/helpers/helpers.v1.d/ruby

Line 285 in f7ed895

local installed_apps=$(yunohost app list | grep -oP 'id: \K.*$')

alexAubin · 2025-01-10T22:31:51Z

helpers/helpers.v1.d/go

-    HOME=$install_dir eval "$(goenv init -)"
+    export HOME=${HOME:-"/root/"}
+    eval "$(goenv init -)"


I would tend to keep the previous syntax such that HOME doesn't get change in the rest of the script which could have side effect idk ... also we probably want to make sure that HOME is the app's install dir and not /root/ ?

I think we should remove :
the part :

yunohost/helpers/helpers.v2.1.d/go

Lines 150 to 159 in cd12849

# Set environment for Go users

echo "#goenv

export GOENV_ROOT=$GOENV_INSTALL_DIR

export PATH=\"$GOENV_INSTALL_DIR/bin:$PATH\"

eval \"\$(goenv init -)\"

#goenv" > /etc/profile.d/goenv.sh

# Load the environment

export HOME=${HOME:-"/root/"}

eval "$(goenv init -)"

from each helper Go and Ruby; helper 2 and helper2.1

The first part /etc/profile.d/**** is to set the environment variables at startup of the Bash shell. But we don't need go and ruby environment variables set to each users.

The second part eval \"\$(goenv init -)\" is to initialise environment for current user, but we don't need that either, as all we need is done in _ynh_load_go_in_path_and_other_tweaks same for ruby

helpers/helpers.v1.d/go

alexAubin · 2025-01-10T22:37:18Z

helpers/helpers.v1.d/go

 export GOENV_ROOT="$goenv_install_dir"
+export goenv_root="$goenv_install_dir"


(are we sure that having that same var in lowercase is actually useful for anything x_x ... i guess it comes from copypasting the same stuff as for ruby but that sounds very meh)

I don't think, it's usefull, but was in ruby helper

alexAubin · 2025-01-10T22:46:23Z

helpers/helpers.v2.1.d/go

-    # Get the absolute path of this version of go
-    go_dir="$GOENV_INSTALL_DIR/versions/$app/bin"
+    # Get the absolute path of this version of Go
+    go_dir="$GOENV_INSTALL_DIR/versions/$go_version/bin"


I'm super confused because it is $app for the ruby helper x_X Or is the ruby helper also wrong ? https://github.com/YunoHost/yunohost/blob/dev/helpers/helpers.v2.1.d/ruby#L30

I don't understand what defines the name of these folder ... I assume they are created when calling rbenv install ? X_x

Ruby helper use the alias plugins :

yunohost/helpers/helpers.v2.1.d/ruby

Line 126 in f7ed895

if git remote -v 2> /dev/null | grep "https://github.com/tpope/rbenv-aliases.git"; then

and

yunohost/helpers/helpers.v1.d/ruby

Line 184 in f7ed895

if git remote -v 2> /dev/null | grep "https://github.com/tpope/rbenv-aliases.git"; then

That plugins is not available for goenv, so there is no $app in $GOENV_INSTALL_DIR/versions/

alexAubin · 2025-01-10T22:49:23Z

helpers/helpers.v1.d/go

-        ynh_print_info --message="Downloading goenv..."
-        git init -q
-        git remote add origin https://github.com/syndbg/goenv.git
+    goenv="$(command -v goenv $goenv_install_dir/bin/goenv | grep "$goenv_install_dir/bin/goenv" | head -1)"


To me what's troublesome is : what is it doing in the first place x_X

It looks like this is meant to verify that $RBENV_INSTALL_DIR/bin/rbenv exists (and maybe is executable) ? Shouldn't we just use something like

rbenv="$RBENV_INSTALL_DIR/bin/rbenv" if [[ -x "$rbenv" ]] then ... else ... fi

(In fact that was the original code x_x)

alexAubin · 2025-01-10T22:51:33Z

(Now i'm just super confused about what's the best starting point between ruby and go, but it definitely looks like the ruby helper code could use some simplifications too @_@)

Co-authored-by: Alexandre Aubin <[email protected]>

yalh76 · 2025-01-12T18:14:11Z

(Now i'm just super confused about what's the best starting point between ruby and go, but it definitely looks like the ruby helper code could use some simplifications too @_@)

I don't know either.

The point is: Go helper was a copy/pasta of Ruby as experimental helper.
goenv is like a fork of rbenv
So both helper should be relativly the same: that was the case when you implemented 2.1 helpers

Limezy · 2025-01-24T08:57:20Z

Hi all, thanks for your work on the matter !
Do you have any visibility on what is blocking this PR to move further ?
There are a few apps left in a broken state for installation as of now (at least Headscale and Outline, but maybe a few others)
I wish I could help but I'm unfortunately very limited in my knowledge of go (and Yunohost helpers mechanisms)
Thanks !

Improving go helper

85fff7c

yalh76 requested a review from alexAubin December 29, 2024 04:50

This comment was marked as resolved.

Sign in to view

Limezy mentioned this pull request Jan 1, 2025

Installation not possible in Yunohost 12.0.9.1 YunoHost-Apps/dex_ynh#67

Open

Merge branch 'dev' into go_helper

d5820fb

Salamandar reviewed Jan 5, 2025

View reviewed changes

Update go

f382928

yalh76 requested a review from Salamandar January 9, 2025 22:36

alexAubin reviewed Jan 10, 2025

View reviewed changes

Update helpers/helpers.v1.d/go

cd12849

Co-authored-by: Alexandre Aubin <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improving go helper #2023

Improving go helper #2023

yalh76 commented Dec 29, 2024

This comment was marked as resolved.

This comment was marked as resolved.

Salamandar Jan 5, 2025

yalh76 Jan 5, 2025

alexAubin Jan 10, 2025

alexAubin Jan 10, 2025

yalh76 Jan 12, 2025

alexAubin Jan 10, 2025

yalh76 Jan 12, 2025

alexAubin Jan 10, 2025

yalh76 Jan 12, 2025 •

edited

Loading

alexAubin Jan 10, 2025

yalh76 Jan 12, 2025

alexAubin Jan 10, 2025

alexAubin commented Jan 10, 2025

yalh76 commented Jan 12, 2025

Limezy commented Jan 24, 2025

		local installed_apps=$(yunohost app list --output-as json --quiet \| jq -r .apps[].id)
		local installed_apps=$(yunohost app list \| grep -oP 'id: \K.*$')

	# Set environment for Go users
	echo "#goenv
	export GOENV_ROOT=$GOENV_INSTALL_DIR
	export PATH=\"$GOENV_INSTALL_DIR/bin:$PATH\"
	eval \"\$(goenv init -)\"
	#goenv" > /etc/profile.d/goenv.sh

	# Load the environment
	export HOME=${HOME:-"/root/"}
	eval "$(goenv init -)"

		export GOENV_ROOT="$goenv_install_dir"
		export goenv_root="$goenv_install_dir"

Improving go helper #2023

Are you sure you want to change the base?

Improving go helper #2023

Conversation

yalh76 commented Dec 29, 2024

The problem

Solution

PR Status

How to test

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yalh76 Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexAubin commented Jan 10, 2025

yalh76 commented Jan 12, 2025

Limezy commented Jan 24, 2025

yalh76 Jan 12, 2025 •

edited

Loading