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

Add Golang #2162

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

Add Golang #2162

wants to merge 15 commits into from

Conversation

slashtechno
Copy link
Contributor

@theofficialgman
Copy link
Collaborator

I'm personally not a fan of random scripts uploaded to github for multiple reasons
for one, this script you linked still uses go 1.18.1, a version from March of this year. stable 1.19.2 is out now.

also, I don't think this meets our app elegibility rubric as this is a programming language/compiler and doesn't really have any GUI functionality #185

I'll keep this open for feedback but if it were to be included, we would need to make an exception to the elegibility rubric or change it.

@slashtechno
Copy link
Contributor Author

slashtechno commented Oct 23, 2022

I'm personally not a fan of random scripts uploaded to github for multiple reasons

In regards to security. I linked the script to the current commit to prevent changes to the script having an effect until the install script is manually updated

also, I don't think this meets our app elegibility rubric as this is a programming language/compiler and doesn't really have any GUI functionality #185

I considered this. However, the rubric specifies the following:

Apps that are not designed for everyday GUI users won't be accepted. (command-line file servers, webservers, etc)

Since Pi-Apps has a category for programming, and many individuals utilize Raspberry Pis for programming, I think it could make life easier for individuals who wish to program in Go, or use go build to compile Go programs

@slashtechno slashtechno reopened this Oct 23, 2022
@ryanfortner
Copy link
Collaborator

We do also have other command-line utilities like GitHub CLI, which many mainly GUI-oriented users aren’t likely to use.

@slashtechno
Copy link
Contributor Author

slashtechno commented Oct 23, 2022

In addition, there are other development focused apps available on Pi-Apps, so I think Go has a place in Pi-Apps.

@theofficialgman
Copy link
Collaborator

theofficialgman commented Oct 23, 2022

The precedent has not been set for compilers to be included in pi-apps. Everything in the programming category is an IDE and similar tooling.

We would need a precident set that we would like CLI only code compilers to be included in the programming category. go is the same in this sense as rustc, clang, gcc, etc and these are not part of pi-apps as "apps"

I personally think that any developers looking to write and compile go code already know the resources for how to follow a two step process to install their tooling from google https://go.dev/doc/install

same goes for rust https://www.rust-lang.org/learn/get-started

@slashtechno
Copy link
Contributor Author

Okay, makes sense

@CleanMachine1
Copy link
Contributor

Of course the version in the debian repos is outdated, why not just skip the middle man here?
The installation according to the docs is just unzipping the binary and deleting the old.

https://go.dev/dl/ https://go.dev/doc/install

This way it can be updated easier and doesn't depend on any script + checksums can be used since they are supplied.

@slashtechno
Copy link
Contributor Author

That's a fair point. However, to install Go, you also need to set up your PATH manually.

@ryanfortner
Copy link
Collaborator

@slashtechno I do not like the idea of a script running within another script, which is what is going on here. If I were you, I would adapt canha's script into a simpler and shorter one.

@slashtechno
Copy link
Contributor Author

@ryanfortner How do you suggest shortening it?

@ryanfortner
Copy link
Collaborator

Maybe just simply splitting install into separate install-32 and install-64 files, then creating a script based on the official Go installation instructions. https://go.dev/doc/install.

@slashtechno
Copy link
Contributor Author

@ryanfortner Okay, will do
I'll try to get the script done this weekend. It'll be hard for me to test it however, since I don't currently have access to my Pi 4.

@ryanfortner
Copy link
Collaborator

I am willing to help out if you need assistance. Just ping me if so.

@slashtechno
Copy link
Contributor Author

@ryanfortner Is this better? I attempted to get rid of functions which won't be used in the script

apps/golang/install Outdated Show resolved Hide resolved
apps/golang/install Outdated Show resolved Hide resolved
apps/golang/install Outdated Show resolved Hide resolved
@@ -0,0 +1,4 @@
(github.com/.../...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To signify that the following lines are Github links in the form of username/repo

Copy link
Collaborator

Choose a reason for hiding this comment

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

why not just add them as links directly then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion, it looks cleaner this way.

@slashtechno
Copy link
Contributor Author

@theofficialgman Attempted to resolve the issues in commit cf6e56e

@theofficialgman
Copy link
Collaborator

theofficialgman commented Nov 4, 2022

this script really won't function in pi-apps

pi-apps scripts are always exclusivly run in bash, no matter what the users default shell program is (this is for compatibility reasons). the script checks the currently running shell, which will always return bash in pi-apps

also the uninstall script still has MacOS crap in it.

@slashtechno
Copy link
Contributor Author

pi-apps scripts are always exclusivly run in bash, no matter what the users default shell program is (this is for compatibility reasons). the script checks the currently running shell, which will always return bash in pi-apps

How can this be alleviated?

@theofficialgman
Copy link
Collaborator

actually nevermind, it looks like SHELL is a special varaible containing the default login shell. so if the user changes their default shell, it will be correctly interpreted even if the script is currently running in bash.

uninstall script still needs those macos removal edits though

@theofficialgman
Copy link
Collaborator

I think you need to actually go through and read and understand the scripts. There are quite a few things broken in the uninstall due to missing variables being set from you removing too much.

@slashtechno
Copy link
Contributor Author

I think you need to actually go through and read and understand the scripts. There are quite a few things broken in the uninstall due to missing variables being set from you removing too much.

Apologies, should be fixed now. I should have noticed the missing variable declaration earlier.
I tested the uninstall script, and it seems to be working. I don't have my Raspberry Pi currently, so I am unable to test the install script fully.

@@ -0,0 +1,90 @@
#!/bin/bash
VERSION="1.19.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you change this and the references to lowercase version then our updater will be able to update it at some point as well as our metadata parser will be able to find the app version

Copy link
Collaborator

Choose a reason for hiding this comment

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

also stable version is now 1.19.3, which is why an updater within pi-apps would be necessary

for personal reference, this will get the current version to add to a pi-apps auto updater script
curl -s https://go.dev/dl/?mode=json | jq -r '.[0].version'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made VERSION lowercase, and removed VERSION="1.19.2"
Is that all that was needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

no, version=1.19.3 need to exist there... you weren't supposed to remove it, just change from uppercase to lowercase

@@ -1,5 +1,4 @@
#!/bin/bash
VERSION="1.19.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

uhh.. you removed it outright

apps/golang/install Outdated Show resolved Hide resolved
apps/golang/install Outdated Show resolved Hide resolved
@theofficialgman
Copy link
Collaborator

I suggest reading the create an app wiki page: https://github.com/Botspot/pi-apps/wiki/Creating-an-app

@theofficialgman
Copy link
Collaborator

theofficialgman commented Nov 6, 2022

I would rather have go installed systemwide instead of only for the current user

follow official go instructions https://go.dev/doc/install

create a custom profile.d script to set the path variable to include the system go installation. I suggest naming it /etc/profile.d/go.sh

ubuntu/debian package it slightly differently, avoiding setting the GOPATH and simply creating symlinks from go and gofmt to the correct bin folder... thats probably the best method and avoids touching the profile at all

to be honest there is so much wrong with this installation method that I will probably just rewrite it myself and add you as a co-contributor to the commit message.

@Botspot can you confirm if you would even like this (go/golang) in pi-apps (see here and the following messages #2162 (comment) )

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

Successfully merging this pull request may close these issues.

4 participants