Repository navigation
Conversation
|
Make sense overall but adding a |
|
Thanks — added in 94e69a4.
|
| .PHONY: nowebui | ||
| nowebui: | ||
| go generate | ||
| go install -ldflags "$(LDFLAGS)" . |
There was a problem hiding this comment.
I don't think a target named nowebui should install. If you look at the others, make or make all build the binary locally. make install does the same but also install. You would expect make nowebui to simply build locally.
I think that's fine to not install, and instruct the user to move the binary where needed, like in other parts of the INSTALLATION instructions.
There was a problem hiding this comment.
Done — you're right that nowebui shouldn't install. go build instead of go install, so the target now sits with the others:
| target | needs Node.js/pnpm | tags | result |
|---|---|---|---|
build |
yes | webui |
binary in the working tree |
build/debug |
yes | webui |
binary in the working tree |
install |
yes | webui |
binary in $(go env GOPATH)/bin |
nowebui |
no | none | binary in the working tree |
//:Makefile is now four lines apart in the same shape as build, differing only in dropping build-webui and the tag.
INSTALLATION.md no longer claims the target put the binary anywhere. It says it lands in the working directory like make build does, and ends with the move, using the Go binary directory the surrounding section already tells you to put on your PATH:
mv git-bug "$(go env GOPATH)/bin"
Ran it rather than assuming:
make nowebuiexits 0 on a fresh clone, nonode_modules, nopnpm install../git-bugis present;$(go env GOPATH)/binuntouched (the binary there is from an earlier session, older mtime).git-bug version→v0.11.0-44-g94e69a4-dirty, so the-ldflagsstamping survives.git-bug webui→Error: this binary was built without the web UI+ the explanation.mv git-bug "$(go env GOPATH)/bin"verified to work from a copy.
One thing I checked because the target runs go generate: CI's Check generated files are up to date step regenerates doc/man, doc/md and misc/completion. Those three paths are byte-identical after go generate without the webui tag, so dropping the tag doesn't churn committed output. gofmt -l . clean too, and no Go source changed.
I also corrected a claim in the previous body: it said the version reports undefined and framed the section around bare go install .. That was right for bypassing the Makefile but wrong for the target, which stamps the version — it now documents make nowebui and keeps the undefined caveat scoped to the bypass case.
The 'Build from source' section listed only git, go and make, but the 'install' and 'build' targets in the Makefile run 'build-webui' first, which needs Node.js and pnpm. On a minimal system the documented steps fail at 'make install' with pnpm missing. Also document how to build without the web UI, which is possible because the embedded assets sit behind the 'webui' build tag: 'go install .' on a fresh clone needs nothing beyond git and go, and 'git bug webui' on that binary explains itself instead of serving a broken page. This is already described in CONTRIBUTING.md; INSTALLATION.md now agrees with it rather than contradicting it. Refs git-bug#1578
The install docs previously pointed at bare `go install .`, which skips the Makefile's git-describe ldflags and leaves `git bug version` as undefined. A dedicated nowebui target keeps the same stamping as `make install` without pulling in Node.js/pnpm, so INSTALLATION.md can drop the version caveat.
Following MichaelMure's review on git-bug#1651: a target named `nowebui` should behave like `build`, which leaves the binary in the working tree, and only `install` should install. `nowebui` now runs `go build` instead of `go install`, so it lines up with `build` and `build/debug` and differs from them only in dropping `build-webui` and the `webui` tag. INSTALLATION.md now says so, and tells the reader to move the binary onto their PATH (the Go binary directory the surrounding section already describes) rather than claiming it is placed there for them. Verified: `make nowebui` exits 0 and leaves ./git-bug; nothing is written to $(go env GOPATH)/bin; the version stamp is unchanged (v0.11.0-44-g94e69a4-dirty); `git bug webui` still prints the no-web-UI message. `go generate` without the tag leaves doc/man, doc/md and misc/completion byte-identical, so the generated-files CI gate is unaffected. `gofmt -l .` is clean and no Go source changed.
94e69a4 to
a27570e
Compare
- `nowebui` is newer than every release tag, so tell readers to check out a revision that carries it instead of leaving `make` to fail with "No rule to make target 'nowebui'" on v0.11.0 and older. - Create the Go binary directory before moving the binary. Without it, `mv git-bug "$(go env GOPATH)/bin"` renames the binary to a *file* named `bin` and exits 0, so the command never lands on PATH. - `asdf install` only supplies versions for already-registered plugins, so point at the registrations in CONTRIBUTING.md.
|
Follow-up on Three review comments landed on
Still docs-only, no Go source changed. |
| `git bug webui`, and needs nothing beyond `git`, `go` and `make`. | ||
|
|
||
| The `nowebui` target is newer than every release tag, so none of them have it: | ||
| checking out `v0.11.0` or older and running `make` fails with `No rule to make |
There was a problem hiding this comment.
Wrong command in release warning On
v0.11.0 or an older tag, bare make runs the default web-UI build. The quoted No rule to make target 'nowebui' error occurs with make nowebui instead. Naming the wrong command could confuse readers trying to understand why the web-UI-free build is unavailable on a release tag.
| checking out `v0.11.0` or older and running `make` fails with `No rule to make | |
| checking out `v0.11.0` or older and running `make nowebui` fails with `No rule to make |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
INSTALLATION.md's Build from source section listedgit,goandmakeas the only dependencies, but the
installtarget that the section tells you torun pulls in the web UI first, and that needs Node.js and pnpm:
So the documented steps fail on a minimal system at
make installwith pnpmmissing. This adds the two missing dependencies to the list, points at
.tool-versionsfor the pinned versions, and answers the second half of theissue: building without the web UI is already possible, it just isn't written
down.
Changes
INSTALLATION.mdand//:Makefile.nodejsandpnpmadded to the dependency list, marked as web-UI-only, witha pointer to the new section below. Also links
.tool-versions(which pinsgolang,nodejs,pnpmandgoreleaser) and theasdf installshortcut,matching how
CONTRIBUTING.mdpoints at the same file.nowebuitarget in//:Makefilethat runsgo generate+go buildwithout thewebuitag, so it needs no Node.js toolchain.entry in the
mdformatTOC. It documentsmake nowebuias the alternative tomake install.nowebuibuilds, it does not installnowebuiisgo build, notgo install, so it is consistent with the othertargets in the file:
buildwebuibuild/debugwebuiinstallwebui$(go env GOPATH)/binnowebuiOnly
installwrites toGOPATH/bin, which is what the surroundingBuild from source section already tells you to put on your
PATH. The newsection therefore ends with a
mvinto that directory rather than claiming thetarget did it for you.
Why building without the web UI works
The embedded assets sit behind a build tag, so the package compiles with an empty
asset set when the tag is absent:
That behaviour is already documented in
CONTRIBUTING.md; this makesINSTALLATION.mdagree with it instead of contradicting it, and gives it thepointer from the issue.
Verification
make nowebuion a fresh clone with nonode_modulesand nopnpm install:Confirmed alongside the binary: nothing was written to
$(go env GOPATH)/bin,and
mv git-bug "$(go env GOPATH)/bin"— the instruction the new section gives —works.
One caveat is called out in the new section rather than left for users to trip
over:
git bug versionreportsundefinedfor a binary built without theMakefile, because
getVersion()in//:version.gorunsgit describeoutputthrough
semver.IsValidafter adding avprefix, so only an exact release tagproduces a version string. Going through
make nowebuikeeps the-ldflagsstamping and reports
v0.11.0-44-g94e69a4-dirtyabove, so this only affectsanyone who bypasses the Makefile.
Because
go generateis part of the target, and the CI Check generated filesare up to date step regenerates
doc/man,doc/mdandmisc/completion:those three paths are byte-identical after running
go generatewithout thewebuitag, so dropping the tag does not churn committed generated output.CI
gofmt -l .clean (no Go source changed).make build-webui, the pnpm test suite andmake secureare untouched.Refs #1578
The PR appears safe to merge; the remaining documentation error is non-blocking.
Findings
Summary
The PR documents the web UI’s Node.js and pnpm build dependencies and adds a
nowebuitarget and installation instructions.Reviews (2) · Last reviewed commit: "docs(install): fix three gaps in the wit..."