maxsupermanhd / FactoCord-3.0

Factorio bidirectional Discord bridge bot for Linux servers with control functionality.
Apache License 2.0
14 stars 10 forks source link

fix: use v3 module import path, fix some lint issues #52

Closed jaredallard closed 2 months ago

jaredallard commented 2 months ago

In order to use go install with a pinned version, we need to follow the Go major version rules and use a v3 import path. This fixes that.

Upgraded dependencies and removed deprecated Go standard libray calls to ioutil.

Cleaned some lint errors I ran into while doing the above.

Lastly, updated CI to use newer actions versions and Go 1.22.

jaredallard commented 2 months ago

Happy to split up any parts of this if wanted, just let me know!

TechnoStrife commented 2 months ago

Please explain why a v3 module path is needed for go install and why we need to do go install?

jaredallard commented 2 months ago

Please explain why a v3 module path is needed for go install and why we need to do go install?

Why go install: It's an easy way to install FactoCord as opposed to downloading a pre-built. For example, if I have a working Go toolchain, I could replace doing:

git clone https://github.com/maxsupermanhd/FactoCord-3.0
git switch v3.2.18
go build -o /an_output_dir/FactoCord-3.0 .

with just this:

go install github.com/maxsupermanhd/FactoCord-3.0/v3@v3.2.18

The go install docs on go.dev are probably helpful here: https://go.dev/ref/mod#go-install

As for why go install doesn't work as-is, because it validates that the import path matches the version. Which as per the major version upgrade docs, is required when doing above v1 versions.

Example:

$ go install github.com/maxsupermanhd/FactoCord-3.0@v3.2.18
go: github.com/maxsupermanhd/FactoCord-3.0@v3.2.18: github.com/maxsupermanhd/FactoCord-3.0@v3.2.18: invalid version: module contains a go.mod file, so module path must match major version ("github.com/maxsupermanhd/FactoCord-3.0/v3")
maxsupermanhd commented 2 months ago

I could replace doing (3 commands that don't even really needed) with just this (go install)

While I dig the idea of go install toolchain I don't think it is better than wgetting or curling prebuilt binary from releases. How many operating systems ship with golang (of modern version like 21/22) preinstalled and how many cloud providers create you servers with such toolchains?

TechnoStrife commented 2 months ago

Have you used FactoCord yourself? Why do we need to install it in GOBIN?

jaredallard commented 2 months ago

To be clear, I'm not trying to say users should be using go install over prebuilts. As a maintainer of a ton of different Go projects myself, I certainly don't expect users to have a Go toolchain 😆

I'm simply trying to have the built-in Go functionality to work as expected for those who do or are in build environments. My use case here is that I'm including Factocord in a Docker image for running Factorio. Previously, this was working for me because I didn't pin versions, but as I'm cleaning it up, I'd like to pin. Pinning forces module aware mode, which this project is not in compliance with right now. This PR would fix that.

maxsupermanhd commented 2 months ago

I'm including Factocord in a Docker image for running Factorio

In your docker image just download the binary and use it, what's the problem? If you are using docker I assume you are redeploying it all the time and updating or whatnot (you guys are like this iirc) then it would be much faster to bootstrap the container with ready-made binary rather than pulling source and building...

I just don't see a point in all of that, why would you care in what language it is written and try to use language-specific toolchain if main method of installation is a prebuilt binary?

jaredallard commented 2 months ago

There's various different reasons for wanting to build from source. Supply chain integrity, reproducible builds, reducing versions of build tools in use (e.g., Go 1.14 vs Go 1.22). I would argue that there shouldn't be documentation/a section for "building from source" if that's not something y'all want to support.

That aside, I want to again emphasize that the change being made here is to have the project follow Go's own requirements for module versioning. It's not specific to making go install work.

jaredallard commented 2 months ago

Anyways, we don't need to merge this if not desired/wanted. I just wanted to open a PR before I ended up changing the import path to point to my fork, I'm happy to keep these changes on my fork and reevaluate later if required 😄

maxsupermanhd commented 2 months ago

Does go install properly set -ldflags "-X 'github.com/maxsupermanhd/FactoCord-3.0/support.FactoCordVersion=$version'"?

jaredallard commented 2 months ago

Does go install properly set -ldflags "-X 'github.com/maxsupermanhd/FactoCord-3.0/support.FactoCordVersion=$version'"?

It does not. Go has moved towards using debug.ReadBuildInfo to get version info. There's various different ways of getting that information, I've been using https://github.com/caarlos0/go-version to make that work with additional LD flag information. But, long answer short, as-is go install is much like go build, so nothing would be set.