vugu / vugu

Vugu: A modern UI library for Go+WebAssembly (experimental)
https://www.vugu.org
MIT License
4.8k stars 175 forks source link

Install golangci-lint with deb package instead of building it #258

Closed Lord-Y closed 6 months ago

Lord-Y commented 6 months ago

Hello @bradleypeabody @owenwaller ,

I'm just changing the way of installing golangci-lint to avoid golang version issues. It's related to https://github.com/vugu/vugu/pull/252#issuecomment-1826965915 for go 1.17.x

Lord-Y commented 6 months ago

During the unit testing for golang 1.17.x, the operation is cancelled with no obvious reasons. Do you guys have a clue or can you guys relaunched it?

owenwaller commented 6 months ago

Hi @Lord-Y

Sorry this change doesn't work.

You cannot assume anything about the host platform, so you change will fail on any non-debian Linux, so builds Redhat and SESU will fail as well as Windows and Max OS.

The go install of golangci-lint is the only safe cross platform way to do this.

Can please close this PR?

Thanks

Lord-Y commented 6 months ago

@owenwaller According to the github workflow, all testing are running on ubuntu so yes it will work. Tinygo was also installed with a deb package.

owenwaller commented 6 months ago

Hi @Lord-Y

Sorry, I missed it was in the .github workflow file. I've asked Brad about something similar recently (building using a Makefile) and the conclusion was no, we want cross platform builds when building vugu locally.

What are you trying to achieve with this change?

If you trying to get to a repeatable build (which I'd agree is a good thing) which we won't get if we use the @latest version of golangci-lint then we can swap that for an @v1.55.2 and it'll have the same effect, still using an go install.

I just can't see why we need the .deb file. Whats the advantage to the project over the go install approach?

The TinyGo install, I've not looked at to be honest. I though it was via docker, in the workflow file?

Lord-Y commented 6 months ago

@owenwaller in the PR 252, we have error with golang 1.17.x related to golanci-lint. Building golangci-lint require golang 1.18 minimum. So instead of building it, install the provided version is better an will always work when using lower versions of golang.

owenwaller commented 6 months ago

@Lord-Y Ah ok now I get it.

Sorry I tend to move with the go tool chain, so I never see things like trying to build tools and libraries with a older and unsupported version of go. I don't think we should be doing anything that supports building or testing vugu on an unsupported version of go.

In which case I have an alternative suggestion. Rather than make this change on its own can we revert to the go install method, but do the version fixing (if that is also important) in PR #257?

That way golangci-lint will only be downloaded, build and installed with a go v1.21.X tool chain which it does support.

Does that work?

bradleypeabody commented 6 months ago

See comment, but seems okay to me, merging.