docs: include required tools in source tree by thaJeztah · Pull Request #4903 · docker/cli

thaJeztah · 2024-02-29T12:18:20Z

carry of docs: include required tools in source tree #4228
closesdocs: include required tools in source tree #4228
⚠️ we should update this to use Man support cli-docs-tool#14, so that we don't have to compile and install go-md2man

In order to be able to build the documentation without internet access (as is required by some distribution build systems), all of the source code needed for the build needs to be available in the source tarball.

This used to be possible with the docker-cli sources but was accidentally broken with some CI changes that switched to downloading the tools (by modifying go.mod as part of the docs build script).

This pattern also maked documentation builds less reproducible since the tool version used was not based on the source code version.

Fixes: commit 7dc35c0 ("validate manpages target")
Fixes: commit a650f4d ("switch to cli-docs-tool for yaml docs generation")

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2024-02-29T12:19:55Z

(Also wondering if we can rewrite things to be a go:generate)

codecov-commenter · 2024-04-12T11:30:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.43%. Comparing base (b199ece) to head (535bb6c).
Report is 12 commits behind head on master.

Additional details and impacted files

@@ Coverage Diff @@## master #4903 +/- ##
=======================================
Coverage 59.43% 59.43% =======================================
Files 358 358 Lines 29765 29765 =======================================
Hits 17690 17690 Misses 11111 11111 Partials 964 964

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

thaJeztah · 2025-03-19T12:17:08Z

-//go:build tools
-
-package main
-
-import (
-	_ "github.com/cpuguy83/go-md2man/v2"
-)


I removed the tools.go here, as we already vendor the dependency as indirect dependency of docker/cli-docs-tool. I could keep if it that's a concern (but things would show up pretty fast if we'd miss the dependency at some point)

crazy-max · 2025-03-19T14:59:21Z

 	github.com/cenkalti/backoff/v4 v4.3.0 // indirect
 	github.com/cespare/xxhash/v2 v2.3.0 // indirect
 	github.com/containerd/log v0.1.0 // indirect
+	github.com/cpuguy83/go-md2man/v2 v2.0.6 // indirect


We could make use of the new tool directive starting Go 1.24:
https://go.dev/doc/modules/managing-dependencies#tools
https://go.dev/ref/mod#go-mod-file-tool

Ah, yes, but we're not on go1.24 yet 😅
At least currently it works, so I guess it's fine with how we have it right now?

thaJeztah · 2025-03-19T18:25:14Z

Let me move this one out of draft; I have a follow-up to rewrite some code to use more of the man-page code from cli-docs-tool, but didn't want to stuff that in with this PR.

tianon-sso · 2025-03-20T20:15:29Z

+	tee "${ROOTDIR}/go.mod" >&2 <<- EOF
+ module github.com/docker/cli
+
+ go 1.23.0
+	EOF


Should/could this instead be something like grep -E '^(module|go) ' vendor.mod | tee go.mod so there's less to dual-maintain?

Oh! Didn't do this one yet, might do so in a follow-up

tianon-sso · 2025-03-20T20:21:14Z

-}
+update() (
+ set -x
+ "${SCRIPTDIR}"/with-go-mod.sh go mod tidy -modfile vendor.mod


Again, I think it's better to document that the invoker of these should run with-go-mod.sh, especially since that'll cause less churn if there's more than one usage of go.mod within a given script.

❤️ your comments made me realise that I don't have to include it in each script; I moved it to the Makefile instead; it's not perfect, but at least keeps the scripts themselves clean. Possibly we could move it into a Makefile var / macro as well so that it can be overridden (to run the scripts without the with-go-mod.sh script before it).

In order to be able to build the documentation without internet access (as is required by some distribution build systems), all of the source code needed for the build needs to be available in the source tarball. This used to be possible with the docker-cli sources but was accidentally broken with some CI changes that switched to downloading the tools (by modifying go.mod as part of the docs build script). This pattern also maked documentation builds less reproducible since the tool version used was not based on the source code version. Fixes: 7dc35c0 ("validate manpages target") Fixes: a650f4d ("switch to cli-docs-tool for yaml docs generation") Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2025-03-20T23:27:06Z

-	_ "github.com/spf13/cobra/doc"
-	_ "github.com/spf13/pflag"
-)
+import _ "github.com/cpuguy83/go-md2man/v2"


Turned out I needed the tools.go after all (don't ask!) 🙈 😂

thaJeztah · 2025-03-20T23:31:00Z

+if [ ! -x "$(command -v $GO_MD2MAN)" ]; then
+ (
+ set -x
+ go build -mod=vendor -modfile=vendor.mod -o ./build/tools/go-md2man ./vendor/github.com/cpuguy83/go-md2man/v2


Now that we have this check; perhaps I should even consider using a regular go install for this. Packagers that want to build offline can pre-install go-md2man if needed I guess?
I'll leave that as a follow up I think

tianon-sso

Nice, much better (just a couple really minor nits but overall LGTM ❤️)

Use the same script as is used in moby/moby, which more gracefully handles an existing `go.mod` (which can be symlinked) into account. - keep the scripts called generic, and update the Makefile to invoke them with the "with-go-mod.sh" script. - use "go run" instead of building temporary binaries - check if go-md2man exists before building a binary Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2025-03-21T13:04:05Z

@Benehiko @vvoland PTAL

I'll have a look at some follow-ups as well;

Rewrite the manpage generator to make use of Man support cli-docs-tool#14
☝️ I realized that won't (currently) replace go-md2man, but as we're already using code from that, we may be able to include its functionality (?)
👉 because effectively, it's ~20 lines of code or less to execute its "main" command; https://github.com/cpuguy83/go-md2man/blob/ae8f92a626e8922695dbf0fb24cf0241e056082c/md2man.go#L31-L48
😄 possibly rewrite this code to a //go:generate, so that we won't need any of the (ba)sh scripts at all, and just go generate ./... to generate or update these files.

thaJeztah force-pushed the carry_docs_no_internet branch from f6f0e4b to b46bfe4Compare April 12, 2024 11:29

thaJeztah mentioned this pull request May 23, 2024
docs: include required tools in source tree #4228
Closed

thaJeztah force-pushed the carry_docs_no_internet branch from b46bfe4 to bc126aaCompare May 23, 2024 20:45

tianon mentioned this pull request May 23, 2024
Update to Docker 23 tianon/debian-moby#13
Merged

thaJeztah force-pushed the carry_docs_no_internet branch from bc126aa to 1ff8868Compare March 19, 2025 12:12

thaJeztah commented Mar 19, 2025
View reviewed changes

thaJeztah force-pushed the carry_docs_no_internet branch from 1ff8868 to 1c298b1Compare March 19, 2025 14:45

crazy-max reviewed Mar 19, 2025
View reviewed changes

thaJeztah marked this pull request as ready for review March 19, 2025 18:25

thaJeztah requested a review from a team as a code owner March 19, 2025 18:25

thaJeztah requested a review from tianon-sso March 19, 2025 18:25

tianon-sso reviewed Mar 20, 2025
View reviewed changes

thaJeztah force-pushed the carry_docs_no_internet branch from 1c298b1 to 8eb269fCompare March 20, 2025 23:20

thaJeztah commented Mar 20, 2025
View reviewed changes

tianon-sso approved these changes Mar 20, 2025
View reviewed changes

Comment threadscripts/docs/generate-man.sh Outdated
Comment threadscripts/docs/generate-man.sh Outdated

thaJeztah force-pushed the carry_docs_no_internet branch from 8eb269f to 535bb6cCompare March 20, 2025 23:37

tianon-sso approved these changes Mar 20, 2025
View reviewed changes

thaJeztah added kind/docs status/2-code-review kind/refactor PR's that refactor, or clean-up code area/packaging labels Mar 21, 2025

thaJeztah added this to the 28.0.3 milestone Mar 21, 2025

thaJeztah requested a review from vvoland March 21, 2025 13:17

thaJeztah requested a review from Benehiko March 21, 2025 13:17

vvoland approved these changes Mar 21, 2025
View reviewed changes

thaJeztah merged commit 61cd986 into docker:masterMar 21, 2025

thaJeztah deleted the carry_docs_no_internet branch March 21, 2025 17:49

thaJeztah mentioned this pull request Mar 25, 2025
man: rewrite to use cli-docs-tool manpage generator #5966
Merged

Uh oh!

Conversation

thaJeztah commented Feb 29, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Feb 29, 2024

Uh oh!

codecov-commenter commented Apr 12, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 19, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tianon-sso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thaJeztah commented Mar 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

thaJeztah commented Feb 29, 2024•
edited
Loading

codecov-commenter commented Apr 12, 2024•
edited
Loading