Linting code and upgrade librairies in preparation for v1.0.0

[Lord-Y]

Hello @bradleypeabody ,

Following the issue https://github.com/vugu/vugu/issues/246 I decided to give some help.

I added golang 1.21.3 support and I thing we should drop older versions. I also lint the code with golangci-lint in order to have a clean code. Some functions/variables were not used so I had the flag //nolint. We can later delete them. However, the are 2 points of attention:

• https://github.com/Lord-Y/vugu/blob/master/mod-check-default.go#L244 I'm not sure what to do there as reflect.SliceHeader is deprecated
• https://github.com/Lord-Y/vugu/blob/master/tinygo-dev/testpgm.go#L21 I'm not sure if we really need jr.

So far, all tests are passing except https://github.com/Lord-Y/vugu/blob/master/simplehttp/simple-handler_test.go#L46 I feel like all those updates are necessary for a v1.0.0.

Waiting for your feedback.

[Lord-Y]

@bradleypeabody simplehttp unit testing are now passing cf commit https://github.com/vugu/vugu/pull/248/commits/9b79df2a180a43746382c296bc45ec7ef39a6588

You can check why here: https://github.com/golang/go/blob/go1.21.3/misc/wasm/wasm_exec.js#L95

All tests are now OK for golang 1.21.3.

[Lord-Y]

I think you need to allow users to perform unit testing for a better visibility as we will all see the results.

[owenwaller]

@Lord-Y

Thanks of putting the work in. I think this is all stuff we have to do.

This is really one for @bradleypeabody to look at as he knows the code :smiley: rather than me.

Quick minor things that I see:

1) You have added a "sudo" into the WASM tests shell script. Personally I would not do this because this will pause the script and ask for a password. That doesn't work in an automated CI/CD solution where there is no human to type in the password. Also it suggests to me that your local docker is mis-configured. Your local user should be in the docker group AFAIK. 2) I'd personally rather remove than mask out unused functions. The less noise the better. 3) I'm not sure why you want to disable the buildvcs info in the binary. I've had to use that recently to verify a binary was what I thought it was. So I would rather leave this in as its useful. 4) Why does the wasm test timeout need to be 30m and not 20m? That makes me nervous because it is likely to mask issues, rather than show them in my experience.

[Lord-Y]

@owenwaller answers below:

@Lord-Y

Thanks of putting the work in. I think this is all stuff we have to do.

This is really one for @bradleypeabody to look at as he knows the code 😃 rather than me.

Quick minor things that I see:

1. You have added a "sudo" into the WASM tests shell script. Personally I would not do this because this will pause the script and ask for a password. That doesn't work in an automated CI/CD solution where there is no human to type in the password. Also it suggests to me that your local docker is mis-configured. Your local user should be in the docker group AFAIK.

Usually online CI/CD allow root access with sudo. If you check in the master branch https://github.com/vugu/vugu/blob/master/.github/workflows/go.yml#L29 you can see that @bradleypeabody is already using it.

2. I'd personally rather remove than mask out unused functions. The less noise the better.

Agreed and that's what I always do personally and professionally. While going on all files, I also saw a lot of commented functions or lines that needs to be removed too. I just masked these onces for now in order to know what @bradleypeabody wants to do.

3. I'm not sure why you want to disable the buildvcs info in the binary. I've had to use that recently to verify a binary was what I thought it was. So I would rather leave this in as its useful.

Alright, I check how to make this works with it. During all my tests, builds was failing because of it.

4. Why does the wasm test timeout need to be 30m and not 20m? That makes me nervous because it is likely to mask issues, rather than show them in my experience.

In all generated files like *_vgen.go, you will see:

vgn = &vugu.VGNode{Type: vugu.VGNodeType(3), Namespace: "", Data: "div", Attr: []vugu.VGAttribute{vugu.VGAttribute{Namespace: "", Key: "id", Val: "t0"}}}

That's not an error, but your IDE will tell you that []vugu.VGAttribute{vugu.VGAttribute is redundant.

So in order to make things clean, I just add this function https://github.com/vugu/vugu/blob/08480b1dd4270c026f5bfcb0db3f7a1428ed7f06/gen/parser-go.go#L189 the remove redundant definitions. After this, go test -timeout=20m ./wasm-test-suite is taking 25min instead of 20 initially. So no worries.

[Lord-Y]

@owenwaller I removed the sudo commands that I added in commit https://github.com/vugu/vugu/pull/248/commits/8904107e1f61499054a5ed7064fc9c3e5b35bbf8

[Lord-Y]

@owenwaller I fixed the vcs issues on my side, so I removed buildvcs flags in the code. With my last 2 commits, wasm-test-suite are passing in 16min so I switched back the config to 20min.

[Lord-Y]

@bradleypeabody any news? :-)

[bradleypeabody]

@Lord-Y and @owenwaller Sorry for the slow replies. Are you on the Gopher's slack? (I think the link to get in is this: https://invite.slack.golangbridge.org/ ) I'm Brad Peabody on there, and if you can drop me a line there on the key things you want me to look at, I think that could be a good way to manage things. As it stands I just get a huge stream of github notifications in my email and I don't have any easy way to separate by priority.

That said, on this linting PR, my general reaction is that it's way easier to review and merge things that are very focused on specific fixes. On something like this, I just cannot easily tell if all of these changes are totally fine, or if there's some subtle thing that is being missed. I'm not opposed to doing a linting pass at some point, but I just feel like it would be better to try to get a flow going where we have changes that are as narrow as possible and focused on the most critical issues on the way to v1.0

[owenwaller]

@Lord-Y Thanks for all of this, it is much appreciated.

But as @bradleypeabody suggests, could you rearrange this into a series of single commits that fix specific things?

From a maintainers point of view its much easier to review smaller well defined commits, merge them in, and critically merge them out if needed.

A quick glance at the commit titles, suggests there could be commits that :

a) upgrade go to 1.21.X that will also bring in the necessary ioutil changes as well. b) removed the other depreciated functions in the code base c) fixes the WASM tests d) fixes the any linter errors

etc.

You should be able to do this by using git to rearrange the commits for you, and opening new PR's for each change. If you can't I can try syncing your PR and doing if you think this is good to go.

As you have bumped the version of TinyGo to v0.3.0, what happened? Did all of the TinyGo tests pass? See #251 as I've had a request to bump the TinyGo support (see #182). If you can comment under #182 that would be helpful.

[Lord-Y]

@owenwaller @bradleypeabody I'll close this bulk PR and open a series of new ones.