Closed owenwaller closed 2 weeks ago
Thanks for this @owenwaller, awesome. I left one minor comment, but overall I'm good with this approach and I like having the taskfile be the main place where everything lives. The PR/ branch functionality seems awesome and very useful.
My general thoughts on this are:
vugubuild-helper
utility that had these things in it, we'd be set. If you agree, I believe i can find the time to help hammer this out, it seems mostly straightforward to me.Let me know what you think.
Hi @bradleypeabody
Thanks for this.
Ah, yes sorry there is indeed a cut'n'paste error. I'll fix that in due course.
I think we should try to separate out the process of building and testing the wasm-test-suite
from everything else. So I'd rather look at how we do this. My preference is to break this down so it's a clear series of steps in a Taskfile
(or something similar), rather than as it is now where it is triggered by the go test
command. If we look at this I think it will start to unpick both the devutil
and the wasm-test-suite/...
packages.
But, yes I agree a Docker only version of tinygo
is probably a more sensible idea in the long term. Can we be clear if tinygo
should be able to build vugu
(as in vugugen) itself, and run all of the vugu
tests?
The problem of a live-reload I think is separate. I agree we probably want to keep this as that seems to be what "front end devs" are used to from JavaScript. That I think should use the standard Go compiler and not tinygo
for two reasons.
a) it's much faster and as its being served locally we don't care about binary size
b) We need to find out how much the community using tinygo
. My guess is not very much, but it's worth asking the community. I'll open an issue.
I think there is mileage in supporting tinygo
building and testing prior to production, as the tinygo
compiler is different and not every Go standard library package is supported (net/http
in particular being missing is likely to be deal breaker for most of vugu
s users I would imagine). But I think it has to be a set of manual steps (or a series of Taskfile
steps or similar) that are not executed by default. If someone really wanted to use a the tinygo
compiler as part of a live reload process then I think that's up to them to sort out a compile/copy/browser refresh script of some sort. If it turns out that everyone is using tinygo
then look at this again.
However I am very wary of going the route of writing a set of go tools (or adding functionality to vugu
even in terms of a new external tool) to support cross platform building. That is more code for us to support and maintain for an solution that already exists on each platform. I can absolutely see where you are coming from however.
If we only use Docker for tinygo
and golangci-lint
then pretty much everything in the bash shell scripts is redundant I think. In particular we are no longer concerned with checking if these tools are locally installed or at what version or what the platform package manager is. This is really the hard part because there is no easy way to convert the bash
shell magic that pulls out the version numbers from the commands into something that Taskfile
could parse. If I move the existing Taskfile
over to only using Docker we might have a better idea of what platform specific bits are left.
If I was going to go down the route of writing OS portable commands, then I think at this stage I'd give serious consideration to using a Magefile
and mage
as the built tool - rather than a Taskfile
as a Magefile
is Go anyway. That way we can either reuse any Go packages (is there a Go grep for example?) rather than write a new vugu
tool that does this. I don't know if any "live reload" functionality could be included in the Magefile or not.
As and end user will have to install the task
command the run the Taskfile
, I don't see that any different from installing mage
to run a Magefile
.
Quickly on goimports
, form out slack
conversation. I would much much rather say that to build vugu
locally you need goimports
already installed. For most developers I think this is the case. I'd then leave an explicit install of goimports
in the Github Action workflow file. If we don't then we are going to have to do something similar to the shell scripts for tinygo
and golangci-lint
where we need to check if goimports
is locally installed, at what version and then download and install it as required. Or use a Dockerzsed goimports
. I don't think either approach is worth it. So I'd rather just say goimports
has to be installed.
Thoughts?
Owen
By way of an experiment, I have tried moving over to using both the Dockerized versions of both tinygo
and golangci-lint
.
To keep this simple its in a new branch in my fork - PR/taskfile_docker_exp
https://github.com/owenwaller/vugu/tree/PR/taskfile_docker_exp
Hopefully it's easier to see the changes if you diff the two branches.
The good news is that it's possible, the Taskfile
is simpler and we don't need the bash
shell scripts any more.
Overall the build is faster, but that's because all of the tests that use a locally installed tinygo
have been commented out.
The bad news is the golangci-lint
linting step is a lot slower - about 40sec from something that was a few seconds.
Most of this seems to be the starting the docker container, inc. building the network bridge which seems to be necessary for the container to run golangci-lint
. I tried disabling it and got a build error from it, but it looks like at some point it is pulling down packages.
It runs just fine as can be seen here:
https://github.com/owenwaller/vugu/actions/runs/8791130028/job/24124676652
But I think this is generally an improvement over the PR in this branch.
Can you please try an sync my PR/taskfile_docker_exp
branch and tell me if it works for you on MacOS?
https://github.com/owenwaller/vugu/tree/PR/taskfile_docker_exp
If it does, then I propose that I merge in into this PR as I think it's a step forward.
Thoughts?
great work guys !!!
Task files are really nice way to go.
@gedw99 Don't get too excited just yet, this is still just very much an idea. So the Taskfile
may or may not stay.
It has advantages, in that's a bit clearer than a Makefile, but its not as cross platform as it looks (esp. for Windows) and I've already run into issues trying to write shell in YAML!
Things might stay the same or they may change to a wholly Go based approach, as per the discussion above.
I realize we're still going back and forth on the best way to do this, but just FYI I tested this on MacOS and task all
gets to the point of calling download_golangci_lint.sh and then fails:
task: "all" started
task: "print-taskfile-version" started
task: [print-taskfile-version] task --version
Task version: v3.13.0 (h1:H1LzM7Ac2Ixz26086sgN7xxueXHcjVwy7qt9BrboiTs=)
task: "print-taskfile-version" finished
task: "clean-auto-generated-files" started
task: [clean-auto-generated-files] find ./wasm-test-suite -type f -name "*_gen.go" -exec rm {} \;
task: "clean-auto-generated-files" finished
task: "build" started
task: [build] go install github.com/vugu/vugu
task: [build] go install github.com/vugu/vugu/cmd/vugugen
task: [build] go install github.com/vugu/vugu/cmd/vugufmt
task: [build] go generate github.com/vugu/vugu/vgform
task: "build" finished
task: "download-latest-golangci-lint" started
task: [download-latest-golangci-lint] ./download_golangci_lint.sh
/Users/bradgp/git/vugu-ow/download_golangci_lint.sh: line 12: jq: command not found
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 14703 100 14703 0 0 46018 0 --:--:-- --:--:-- --:--:-- 45946
curl: Failed writing body
The lastest release of golangci-lint is
GOLANG_CI_LINT_EXE
No golangci-lint installed locally. Downlaoding and installing golangci-lint
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 9 100 9 0 0 33 0 --:--:-- --:--:-- --:--:-- 33
sudo: dpkg: command not found
task: "download-latest-golangci-lint" finished
task: "lint" started
task: glangci-lint not found. Please see https://golangci-lint.run/usage/install/#local-installation for how to install locally
task: Failed to run task "all": task: precondition not met```
Hey all.
I know what you mean about interoperability of the calls inside the task files.
one approach is to use a golang tool that is interoperable that the task file calls. That golang binary does all the real work.
It works well :) it can also make sure tinygo is installed , do any file system things you need . Typically a Vugu project will have a few runtime dependencies too like DB etc. So it not nuts to have a golang binary called “ ops” that does this stuff at compile, built, deploy time. Just depends how far you want to take it.
an example of this is TiUp, that is part of the tiDB project. It’s golang and on github and does all these things. It’s just a binary that works in all OS / ARCH
Btw then the local and GitHub CI runs on identical code.
I have a project that does it this way just like what you trying to do.
I can send you the code if you want !
I also want to see if tinygo will crunch the binary size
@bradleypeabody Thanks for trying this.
The shell scripts are even less portable than I thought. Your errors of
/Users/bradgp/git/vugu-ow/download_golangci_lint.sh: line 12: jq: command not found
then later
sudo: dpkg: command not found
are due to the shell script targeting a Debian or a Debian based linux so MacOS doesn't have the jq
Jason query tool (surprising) or the Debian dpkg
package management command (to be expected).
But it's actually worse than this because the curl line (if you look in the script) is downloading the linux/amd64 version of golangci-lint
which won't run on MacOS anyway. Sorry, I missed this point entirely.
Can you do me a 2nd favour? Can you try this again but against my PR/taskfile_docker_exp
branch
https://github.com/owenwaller/vugu/tree/PR/taskfile_docker_exp
Which is the Dockerized version of this PR. I suspect that is more likely to work, if MacOS can run an linux/amd64 container.
That PR removes the shell scripts completely as golangci-lint
and tinygo
are both run in containers.
Anyhow can you give it a go and let me know what happens?
Thanks
@gedw99
one approach is to use a golang tool that is interoperable that the task file calls. That golang binary does all the real work.
Yes, @bradleypeabody and I have had a quick discussion on Slack about just this. So far I am against it, because it's a more code for us to write and maintain, and because it reinvents the wheel. There are already tools that do this, by writing the entire build script (not just the a cross platform tool) in Go. This would mean dropping the Taskfile
based approach.
See: Mage Dagger And I mean the Dagger Engine part.
Either of these will remove the problem, but only if we are careful if and what we call out to via os.Exec
.
I have run into a separate issue using Taskfile
yesterday - were I need it to iterate over the contents of a directory tree to execute go generate
in a set of directories and it's just not working. So I'll need to see if I can resolve this. If I can't then a wholly Go based build is more likely if I can get it to work. Note: This set of changes are not yet public via PR.
@bradleypeabody no, go generate ./...
isn't the solution to this, as go generate
doesn't accept any package arguments. See go help generate
.
Task can call the golang compiled binary .. that’s what I do. Task will install it as part on init ..
On Fri, 26 Apr 2024 at 8:31 PM, Owen Waller @.***> wrote:
@gedw99 https://github.com/gedw99
one approach is to use a golang tool that is interoperable that the task file calls. That golang binary does all the real work.
Yes, @bradleypeabody https://github.com/bradleypeabody and I have had a quick discussion on Slack about just this. So far I am against it, because it's a more code for us to write and maintain, and because it reinvents the wheel. There are already tools that do this, by writing the entire build script (not just the a cross platform tool) in Go. This would mean dropping the Taskfile based approach.
See: Mage https://magefile.org/ Dagger https://dagger.io/ And I mean the Dagger Engine part.
Either of these will remove the problem, but only if we are careful if and what we call out to via os.Exec.
I have run into a separate issue using Taskfile yesterday - were I need it to iterate over the contents of a directory tree to execute go generate in a set of directories and it's just not working. So I'll need to see if I can resolve this. If I can't then a wholly Go based build is more likely if I can get it to work. Note: This set of changes are not yet public via PR.
@bradleypeabody https://github.com/bradleypeabody no, go generate ./... isn't the solution to this, as go generate doesn't accept any package arguments. See go help generate.
— Reply to this email directly, view it on GitHub https://github.com/vugu/vugu/pull/267#issuecomment-2079121579, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMVPLFDGYRX3TLIDSOB4LUTY7IUGVAVCNFSM6AAAAABGQULCBKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZZGEZDCNJXHE . You are receiving this because you were mentioned.Message ID: @.***>
@gedw99
The issue isn't so much "calling" a binary, it's knowing which binary to download and install. And how to find that binary on a projects Github release page. And how to install that binary on a given platform (e.g. do we need dpkg
or yarn
on Linux and what do we need for a BSD?)
If you look at line #25 in the download_golangci_lint.sh
script you'll see the sorts of things we have to get up to.
And this is for a Debian Linux on amd64 hardware. So you can see how the number of possibilities explodes to support all the platforms Go runs on.
Where as if we use the docker version it's much much simpler, as you can see here (in my other branch that is the Dockerized version):
https://github.com/owenwaller/vugu/blob/2e64bbec36b43e2c6a54fb1d8408a2b670384820/Taskfile.yaml#L81
And it's always the same for every platform. I think the image that is pulled is multi-arch so amd64 and arm64.
Which is why i think this'll work on @bradleypeabody Mac :)
So @gedw99 sees this as well....
Take a look at PR #269
https://github.com/vugu/vugu/pull/269
It's proving to be far superior approach.
totally cool with mage too. used it for ages and its nice. you can compile the mage / gaoling code to a bin too. you can use Taskfile to drive mage src or bin... just icing on top of the cake...
This is my GitHub yaml workflow file. I use this for all my projects. it's all I need because my logic is in mage OR tail or Makefile.
it can call anything. Make, Task, Mage works on ALL OS.
the reason I like it is that ALL logic is in my Mage file now. I dont need magic stuff from GitHub actions.
name: make
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true
on:
# Runs on pushes targeting the default branch
push:
branches:
- 'main'
tags:
- 'v*'
# Runs on PRS targeting any branch
pull_request:
# Allows you to run this workflow manually from the Actions tab
workflow_dispatch:
env:
BASE_GWD: ${{ github.workspace }}
BASE_GWD_BIN: ${{ github.workspace }}/.bin
jobs:
#
# Tests for all platforms. Runs a matrix build on Windows, Linux and Mac,
# with the list of expected supported Go versions (current, previous).
#
build:
name: Build and test
strategy:
fail-fast: false
matrix:
os: [ ubuntu-latest, macos-latest, windows-latest ]
go-version: [ 1.22.0 ]
target: [ "ci-all", ]
runs-on: ${{ matrix.os }}
steps:
# Install go: https://github.com/actions/setup-go/releases/tag/v5.0.0
- name: Install go
uses: actions/setup-go@v5.0.0
with:
go-version: ${{ matrix.go-version }}
id: go
# Set go bin
#- name: Setup Go binary path
# shell: bash
# run: |
# echo "GOPATH=${{ github.workspace }}" >> $GITHUB_ENV
# echo "${{ github.workspace }}/bin" >> $GITHUB_PATH
# Fix git line endings
- name: Git line endings
shell: bash
run: |
git config --global core.autocrlf false
git config --global core.eol lf
# Checkout code: https://github.com/actions/checkout/releases/tag/v4.1.1
- name: Check out main code into the Go module directory
uses: actions/checkout@v4.1.1
with:
ref: ${{ github.event.pull_request.head.sha }}
path: ${{ github.workspace }}/go/src/github.com/${{ github.repository }}
# Check workspace
- name: check workspace ${{ matrix.target }}
shell: bash
run: |
echo ${{ github.workspace }}
echo $GITHUB_WORKSPACE
# Build using make
- name: make ${{ matrix.target }}
shell: bash
run: |
make $target
working-directory: ${{ github.workspace }}/go/src/github.com/${{ github.repository }}
env:
# CONFIG_PASSWORD: secretzSoSecureYouWontBelieveIt999
target: ${{ matrix.target }}
# CONFIG_PASSWORD: ${{ secrets.CONFIG_PASSWORD }}
## Test
- name: Test with Go ( fake for noe )
shell: bash
run: touch test-${{ matrix.os }}.json
## Upload other artifacts
- name: upload other
uses: actions/upload-artifact@v4
with:
name: other-${{ matrix.os }}
path: ${{ env.BASE_GWD_BIN }}
## Upload bin artifacts
- name: upload bins
uses: actions/upload-artifact@v4
with:
name: bin-${{ matrix.os }}
path: ${{ env.BASE_GWD_BIN }}
Overview
This PR moves the existing Github Action workflow over to a
Taskfile
based build. The workflow file is now a shim over theTaskfile
.The intent is that a developer or maintainer can have the same build process/experience locally as they do when a push is made to Github.
The Github action is further updated so that it is triggered on a push to any branch named with the prefix
PR/
. This allows the Github action to be triggered in a contributors forked repository as well as in the upstream repository. It is recommended that the this change is communicated to the community is we progress with it.The docs still have to be updated but
vugu
is now built with:task all
Everything else on a Debian based Linux should be automatic.All of the work of building and testing
vugu
is now largely in theTaskfile
.The
Taskfile
will:wasm-test-suite
golangci-lint
to the latest releasewasm-test-suite
tinygo
compiler to the latest release. This is needed to test thewasm-test-suite
.wasm-test-suite
The tests in the
wasm-test-suite
also rely on runningtinygo
in a docker container. This container is always downloaded as part of the tests and always at the latesttinygo
release.The solutions used are not portable. The
Taskfile
and the external scripts that download bothgolangci-lint
andtinygo
all assume a Debian based Linux distribution. This matchesubuntu
that is used by the Github Action runner. These scripts will need to be updated to support MacOS, Windows as well as other versions of Linux and BSD.This PR represents a starting point for discussion for a future build solution for
vugu
. I do no think this PR should be merged in it's current form.Problems
Building
vugu
itself in a cross platform manage is simple, its just Go. The problems are testing and linting the code base.We build
vugu
using the standard Go compiler, but don't build it with any version oftinygo
. It's unclear is this is a mistake or an oversight or was never intended to be the case. This needs to be resolved.The larger issue is with the
devutil
andwasm-test-suite
packagesThe
devutil
package, pulls the latest version oftinygo
in a Docker container. This is not at all obvious. Thedevutil
tests then use the dockerizedtinygo
compiler to see if it can compile a simple Go program, with vugu imports to wasm.The
wasm-test-suite
takes this a step further. It uses three compilers. The standard Go compiler,tinygo
in a docker container and a locally installedtinygo
. All three compilers are managed via thedevutil
package, which thewasm-test-suite
makes use of.For each compiler the the Go test compiles the test code to wasm, serves the wasm from a newly created webserver which itself is running inside a docker container, then tests using a headless browser against the newly started webserver in the container. Before tearing everything down prior to the next test.
The problem with this approach is that it is mixing concerns.
The tests should test the functionality. If that means using a headless browser against a web server in a Docker container that's ok. What the tests should not be doing is acting as a build and deploy onto infrastructure solution, which they currently are doing.
It would be better to separate these concerns out in a separate set of steps e.g.
We can then use the most appropriate tool or tools for each stage.
We also need to separate this process for the standard Go compiler, from the
tinygo
compiler. The later should only be used via Docker as I don't see any reason to a locally installed copy. Thetinygo
compiler isn't any slower under Docker (but it is a lot slower then the standard Go compiler). This would also allow these test sets to run in parallel as opposed to sequentially as is currently the case, resulting in a large reduction in built time as thewasm-test-suite
is anywhere between 10 and 35 minutes.In general we should target the standard Go compiler, first. For a local live reload we are unconcerned about the size of any wasm binary as this will be served locally over a Gbps connection to
localhost.
. I think this should be the default.We need to provide a means to build both
vugu
and anyvugu
based project as wasm usingtinygo
for production use, if the user so wishes. This needs to include running any tests undertinygo
.Achieving this may not be easy, and my require a complete change in tooling (e.g. do we use something like dagger for this (https://dagger.io) or do we use mage as the built tool (https://https://magefile.org/) or do we stick with a
Taskfile
or some combination?Taskfiles
(as this PR shows) are not necessarily portable, if their contents are not also portable. Moving wholly to docker for bothtinygo
andgolangci-lint
may help with this situation.As the test "build" system is already written in Go (as part of the tests themselves) we could move to a wholly Go based build i.e. the build scripts are also Go code, otherwise we need to find a way to make a YAML based build (such as
Taskfile
) work.To goal however should remain the same as this PR. A maintainer or contributor should have the same local build experience as that used by the Github Action - as far as possible - using the same tooling so that there are no surprises when the Github Action runs.
Thoughts?
Owen