Repository navigation
Add urfave_cli_no_template build tag - #2441
Conversation
|
Added the forth commit to modify check-binary-size CI job to show size with the new tag; here's the output: |
|
@ldez @seanamos @abitrolly PTAL |
|
I wanted to create a PR based on the issue but I was waiting for feedback from urfave/cli maintainers before opening it because this is significant change. I will review and test the 2 PRs as soon as possible. |
ee65a8d to
628df91
Compare
Juneezee
left a comment
There was a problem hiding this comment.
Thanks for writing both versions, it makes the trade-off easy to see.
I checked both branches locally with Go 1.27.1:
- Tests pass with and without the tag.
examples/example-cli: 5.58 MB -> 3.28 MB (#2441) and 3.30 MB (#2442).- With the tag,
go build -ldflags=-dumpdep ./examples/example-clishows no<ReflectMethod>symbols (27 without the tag), so DCE is back. - Help output was byte-for-byte identical in both modes for extra commands outside the golden test (wrap widths, flag categories, persistent flags, subcommand
UsageText/Category, pointer and value authors).
Which approach
I prefer #2441. The default templates change a few times a year (9 commits to template.go since 2024). The golden test covers almost all of the hand-written renderer, and the tagged CI run catches drift. Both need a second step when a template changes (go generate vs. a hand edit), so the real difference is the generator itself. It is nicely done, but it adds a 576-line template compiler (internal/genhelp/main.go) and 352 generated lines (help_notemplate_gen.go), where #2441 has one 341-line file of plain Go (help_notemplate.go).
Side note: the two descriptions disagree. #2441 says "I prefer this PR" and #2442 says "my preference is this one". Could you update whichever is stale?
Custom templates fail silently (both PRs)
With the tag, a custom or modified template prints no help, Run returns nil, and nothing is logged unless CLI_TEMPLATE_ERROR_DEBUG is set:
Lines 63 to 73 in 628df91
Lines 361 to 371 in d1d8108
Example:
package main
import (
"context"
"github.com/urfave/cli/v3"
)
func main() {
cmd := &cli.Command{Name: "app", CustomRootCommandHelpTemplate: "NAME: {{.Name}}\n"}
_ = cmd.Run(context.Background(), []string{"app", "--help"})
}go run . prints NAME: app. go run -tags urfave_cli_no_template . prints nothing. The same happens with cli.RootCommandHelpTemplate += "...".
This can bite a binary author who adds the tag without knowing that a dependency customizes the templates. ToFishCompletion already returns errNoTemplate, so help could report it too. Replacing both handleTemplateError(errNoTemplate) calls in DefaultPrintHelpCustom with
_, _ = fmt.Fprintln(ErrWriter, errNoTemplate)works, and both test suites pass on both PRs after skipping TestPrintHelpCustomTemplateError with the tag. That test is about template parse errors. With the tag it only passes because data is not a *Command, so it hits the errNoTemplate path:
Lines 2314 to 2328 in d1d8108
TestNoTemplate_CustomHelpTemplate could then also check the error output.
Nits
TestNoTemplateTagDeps only looks for text/template. Checking the go build -ldflags=-dumpdep ./examples/example-cli output for <ReflectMethod> would also catch other packages that turn DCE off (html/template, a direct MethodByName call). It is cheap, about 0.4s here with a warm cache.
Lines 12 to 20 in 628df91
(#2441 only) The golden test has no subcommand with UsageText, so this branch is never compared against the template:
Lines 142 to 144 in 628df91
Looking for more input from other users and reviewers.
628df91 to
6d1b921
Compare
|
@Juneezee thanks for the thorough review! Addressed everything (rebased on top of main, too):
|
|
@kolyshkin Thanks for the quick update. Everything from the first review is addressed. Verified locally at commit a6313e2:
The fail-early idea in commit a6313e2 is useful, but the current implementation has two drawbacks. 1.
|
a6313e2 to
0443c43
Compare
|
@Juneezee thanks again!
|
0443c43 to
f188851
Compare
|
Update: the template constants with the tag are now just short strings ("root", "command", etc.), since templates are not used in this mode anyway. As a result, the template variables (without the tag) are back to how they were, and the API docs are unchanged from main. |
1d73ed8 to
04c7fd4
Compare
|
Reworked the patchset (and rebased on top of main). Changes since my last comment:
|
Juneezee
left a comment
There was a problem hiding this comment.
@kolyshkin LGTM, thanks! Verified locally at 04c7fd4: tests pass with and without the tag, and with the tag examples/example-cli has no <ReflectMethod> symbols.
Two optional follow-ups, not blocking:
- With the tag, a custom
HelpPrinterreceives"root","command"or"subcommand"astemplinstead of the template text:
Lines 17 to 28 in 04c7fd4
The README line "Replacing cli.HelpPrinter works as usual" could mention that:
Lines 42 to 48 in 04c7fd4
- The
errNoTemplatepanic could include the command name, to make it easier to find which command has the custom template:
Lines 65 to 74 in 04c7fd4
@ldez since the build tag approach was your idea in #2277 (comment), it would be great to hear your thoughts before this is merged.
04c7fd4 to
548718f
Compare
|
@Juneezee thanks! Addressed both:
|
548718f to
b497e28
Compare
|
@ldez ptal 🙏🏻 |
b497e28 to
4b02600
Compare
|
The implementation seem right, I tested it and it works as expected. My only concern is about customization.
This doesn't allow the hack around But this is a detail, we can ignore it for now, at least, I think we cannot provide a better solution without breaking changes. Off topic: I don't think GitHub is able to close automatically and issue when using the word |
Move DefaultPrintHelpCustom, the fish completion template rendering, and the exported templates (RootCommandHelpTemplate etc.) to help_template.go, so that everything which only makes sense with text/template is in one file. No functional change; this is a preparation for the next commit. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
4b02600 to
6107ed5
Compare
I think you are mixing things up here. The command ( There is even a (weak) test, added by this PR, I have now added another test,
Thanks, fixed (although I think @Juneezee manually linked the issue to this PR). |
Using text/template makes the Go linker disable dead code elimination of exported methods for the whole program, since templates can call arbitrary methods by name (see golang/go#72895). For large programs, this results in a noticeably bigger binary. Add urfave_cli_no_template build tag, which renders the default help and fish completion directly, producing the same output, but without using text/template. Custom templates are not supported in this mode. RootCommandHelpTemplate, CommandHelpTemplate, SubcommandHelpTemplate and FishCompletionTemplate are short string constants, only used to tell which help is requested, so an attempt to modify them is a compile error. If the default help printer is used with a custom template, it panics. With this tag, examples/example-cli binary is 3.2 MB instead of 5.1 MB, and runc binary (with opencontainers/runc#5486 applied) is 10.5 MB instead of 12.4 MB. Run tests with this tag in CI, skipping those that use custom templates, and add a test to ensure linker's dead code elimination is not disabled with this tag. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add a test which renders various help pages and fish completion for a few commands exercising most of the template features, and compares the result with a golden file. Since CI runs tests both with and without urfave_cli_no_template tag, this ensures both implementations produce the exact same output. To regenerate the golden file, run the test with UPDATE_GOLDEN=1. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In check-binary-size, also show how much the package contributes to the binary size when built with urfave_cli_no_template tag. This is for informational purposes only; the limits are checked for the default build. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
6107ed5 to
2c876a1
Compare
I missed that a command has a Not easy to use but it can work. |
This PR contains the following updates: | Package | Type | Update | Change | OpenSSF | |---|---|---|---|---| | [github.com/urfave/cli/v3](https://github.com/urfave/cli) | require | minor | `v3.10.1` → `v3.14.0` | [](https://securityscorecards.dev/viewer/?uri=github.com/urfave/cli) | --- >⚠️ **Warning** > > Some dependencies could not be looked up. Check the [Dependency Dashboard](issues/23) for more information. --- ### Release Notes <details> <summary>urfave/cli (github.com/urfave/cli/v3)</summary> ### [`v3.14.0`](https://github.com/urfave/cli/releases/tag/v3.14.0) [Compare Source](urfave/cli@v3.13.0...v3.14.0) #### What's Changed - chore(deps): bump mkdocs-git-revision-date-localized-plugin from 1.5.4 to 1.6.0 in the python-packages group by [@​dependabot](https://github.com/dependabot)\[bot] in [#​2436](urfave/cli#2436) - fix: set BoolWithInverseFlag in a short option group by [@​lazerg](https://github.com/lazerg) in [#​2440](urfave/cli#2440) - Keep arguments after a bare dash by [@​official-burak](https://github.com/official-burak) in [#​2419](urfave/cli#2419) - fix: handle a multi-byte flag at the end of a short option group by [@​SulimanAbdulrazzaq](https://github.com/SulimanAbdulrazzaq) in [#​2438](urfave/cli#2438) - fix: complete persistent flags in shell completion by [@​sb123sb123](https://github.com/sb123sb123) in [#​2437](urfave/cli#2437) - fix: restore Unwrap on the error returned by Exit by [@​fadiroot](https://github.com/fadiroot) in [#​2444](urfave/cli#2444) - fix: reject a parent's Local flag inside a short option group by [@​fadiroot](https://github.com/fadiroot) in [#​2445](urfave/cli#2445) - fix: report a flag that needs an argument but is not last in a short group by [@​yu2971512385-ui](https://github.com/yu2971512385-ui) in [#​2443](urfave/cli#2443) - Use prefixFor so flag suggestions agree with help on rune count by [@​januththedev](https://github.com/januththedev) in [#​2448](urfave/cli#2448) - fix: reject duplicate flag names by [@​deepakganesh78](https://github.com/deepakganesh78) in [#​2396](urfave/cli#2396) - fix: date a time-only timestamp today when Timezone is not UTC by [@​breken-ai](https://github.com/breken-ai) in [#​2450](urfave/cli#2450) - fix: let a subcommand's command-line value replace a persistent flag's env/file value by [@​breken-ai](https://github.com/breken-ai) in [#​2449](urfave/cli#2449) - Add urfave_cli_no_template build tag by [@​kolyshkin](https://github.com/kolyshkin) in [#​2441](urfave/cli#2441) - fix: skip empty source values for flags that cannot parse them by [@​abo3losh1](https://github.com/abo3losh1) in [#​2451](urfave/cli#2451) #### New Contributors - [@​lazerg](https://github.com/lazerg) made their first contribution in [#​2440](urfave/cli#2440) - [@​official-burak](https://github.com/official-burak) made their first contribution in [#​2419](urfave/cli#2419) - [@​SulimanAbdulrazzaq](https://github.com/SulimanAbdulrazzaq) made their first contribution in [#​2438](urfave/cli#2438) - [@​sb123sb123](https://github.com/sb123sb123) made their first contribution in [#​2437](urfave/cli#2437) - [@​fadiroot](https://github.com/fadiroot) made their first contribution in [#​2444](urfave/cli#2444) - [@​yu2971512385-ui](https://github.com/yu2971512385-ui) made their first contribution in [#​2443](urfave/cli#2443) - [@​januththedev](https://github.com/januththedev) made their first contribution in [#​2448](urfave/cli#2448) - [@​deepakganesh78](https://github.com/deepakganesh78) made their first contribution in [#​2396](urfave/cli#2396) - [@​breken-ai](https://github.com/breken-ai) made their first contribution in [#​2450](urfave/cli#2450) - [@​abo3losh1](https://github.com/abo3losh1) made their first contribution in [#​2451](urfave/cli#2451) **Full Changelog**: <urfave/cli@v3.13.0...v3.14.0> ### [`v3.13.0`](https://github.com/urfave/cli/releases/tag/v3.13.0) [Compare Source](urfave/cli@v3.12.0...v3.13.0) #### What's Changed - Trim the key of a map flag when TrimSpace is set by [@​youdie006](https://github.com/youdie006) in [#​2431](urfave/cli#2431) - Fix inverse bool flag counting with an external counter by [@​jakezwang](https://github.com/jakezwang) in [#​2432](urfave/cli#2432) - feat: add Deprecated field to flags and commands by [@​anandghegde](https://github.com/anandghegde) in [#​2429](urfave/cli#2429) - fix: parse single-rune Unicode flags by [@​znnnnnnn-wil](https://github.com/znnnnnnn-wil) in [#​2434](urfave/cli#2434) #### New Contributors - [@​jakezwang](https://github.com/jakezwang) made their first contribution in [#​2432](urfave/cli#2432) - [@​anandghegde](https://github.com/anandghegde) made their first contribution in [#​2429](urfave/cli#2429) - [@​znnnnnnn-wil](https://github.com/znnnnnnn-wil) made their first contribution in [#​2434](urfave/cli#2434) **Full Changelog**: <urfave/cli@v3.12.0...v3.13.0> ### [`v3.12.0`](https://github.com/urfave/cli/releases/tag/v3.12.0) [Compare Source](urfave/cli@v3.11.0...v3.12.0) #### What's Changed - feat: add Command.ArgValidator for tree-wide argument validation by [@​dearchap](https://github.com/dearchap) in [#​2377](urfave/cli#2377) - Single-value arguments can be marked as required by [@​idelchi](https://github.com/idelchi) in [#​2393](urfave/cli#2393) - docs: clarify that named Arguments are excluded from cmd.Args() by [@​dearchap](https://github.com/dearchap) in [#​2411](urfave/cli#2411) - fix: never run command action on shell completion past a double dash by [@​dearchap](https://github.com/dearchap) in [#​2412](urfave/cli#2412) - chore(deps): bump github.com/stretchr/testify from 1.11.1 to 1.12.0 by [@​app/dependabot](https://github.com/app/dependabot) in [#​2413](urfave/cli#2413) - Support custom FlagStringer for MutuallyExclusiveFlags by [@​SHIVANSHGARG07](https://github.com/SHIVANSHGARG07) in [#​2414](urfave/cli#2414) - Add TextFlag for encoding.TextMarshaler/TextUnmarshaler values by [@​ChrisJr404](https://github.com/ChrisJr404) in [#​2415](urfave/cli#2415) - chore(deps): bump github.com/stretchr/testify from 1.12.0 to 1.12.1 by [@​app/dependabot](https://github.com/app/dependabot) in [#​2416](urfave/cli#2416) - chore(deps): bump the python-packages group with 2 updates by [@​app/dependabot](https://github.com/app/dependabot) in [#​2417](urfave/cli#2417) - Show inherited persistent flags in child help by [@​justadityaraj](https://github.com/justadityaraj) in [#​2421](urfave/cli#2421) - fix: preserve whitespace in positional arguments by [@​lenamonj](https://github.com/lenamonj) in [#​2423](urfave/cli#2423) - fix: preserve whitespace in equals-form flag values by [@​BenYang12](https://github.com/BenYang12) in [#​2425](urfave/cli#2425) - fix: complete subcommand flags after positional arguments by [@​fzlzjerry](https://github.com/fzlzjerry) in [#​2426](urfave/cli#2426) - Fix a year-less timestamp layout losing January 1 by [@​youdie006](https://github.com/youdie006) in [#​2427](urfave/cli#2427) - docs: add go-size-analyzer, module version, and reflection by [@​imsumedhaa](https://github.com/imsumedhaa) in [#​2428](urfave/cli#2428) #### New Contributors - [@​idelchi](https://github.com/idelchi) made their first contribution in [#​2393](urfave/cli#2393) - [@​SHIVANSHGARG07](https://github.com/SHIVANSHGARG07) made their first contribution in [#​2414](urfave/cli#2414) - [@​ChrisJr404](https://github.com/ChrisJr404) made their first contribution in [#​2415](urfave/cli#2415) - [@​justadityaraj](https://github.com/justadityaraj) made their first contribution in [#​2421](urfave/cli#2421) - [@​lenamonj](https://github.com/lenamonj) made their first contribution in [#​2423](urfave/cli#2423) - [@​BenYang12](https://github.com/BenYang12) made their first contribution in [#​2425](urfave/cli#2425) - [@​youdie006](https://github.com/youdie006) made their first contribution in [#​2427](urfave/cli#2427) - [@​imsumedhaa](https://github.com/imsumedhaa) made their first contribution in [#​2428](urfave/cli#2428) **Full Changelog**: <urfave/cli@v3.11.0...v3.12.0> ### [`v3.11.0`](https://github.com/urfave/cli/releases/tag/v3.11.0) [Compare Source](urfave/cli@v3.10.1...v3.11.0) #### What's Changed - fix: correct a typo in the help message for the powershell autocompletion by [@​suzuki-shunsuke](https://github.com/suzuki-shunsuke) in [#​2383](urfave/cli#2383) - Escape backslashes in fish completion descriptions by [@​mahirhir](https://github.com/mahirhir) in [#​2378](urfave/cli#2378) - chore(deps): bump actions/setup-go from 6 to 7 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​2389](urfave/cli#2389) - chore(deps): bump mkdocs-material from 9.7.6 to 9.7.7 in the python-packages group by [@​dependabot](https://github.com/dependabot)\[bot] in [#​2391](urfave/cli#2391) - chore(deps): bump actions/setup-python from 6 to 7 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​2390](urfave/cli#2390) - fix: skip all hidden flags in shell completion, not just bool flags by [@​vidigoat](https://github.com/vidigoat) in [#​2388](urfave/cli#2388) - Fix nil pointer dereference for interface-typed flag values by [@​mrueg](https://github.com/mrueg) in [#​2400](urfave/cli#2400) - Fix index out of range panic when Run is given no arguments by [@​mrueg](https://github.com/mrueg) in [#​2401](urfave/cli#2401) - fix: honor BoolWithInverseFlag DefaultText by [@​fzlzjerry](https://github.com/fzlzjerry) in [#​2403](urfave/cli#2403) - Fix Issue [#​2385](urfave/cli#2385) by [@​BrandonIrizarry](https://github.com/BrandonIrizarry) in [#​2386](urfave/cli#2386) - fix: inherit HideHelpCommand in subcommands by [@​utkarshalpha](https://github.com/utkarshalpha) in [#​2395](urfave/cli#2395) - test: cover Run-with-no-arguments paths ([#​2406](urfave/cli#2406)) by [@​dearchap](https://github.com/dearchap) in [#​2407](urfave/cli#2407) - test: add shell-completion case for hidden BoolWithInverseFlag by [@​dearchap](https://github.com/dearchap) in [#​2408](urfave/cli#2408) - test: fix ineffective mutex error-string assertion and add mirror cases by [@​dearchap](https://github.com/dearchap) in [#​2410](urfave/cli#2410) #### New Contributors - [@​mahirhir](https://github.com/mahirhir) made their first contribution in [#​2378](urfave/cli#2378) - [@​vidigoat](https://github.com/vidigoat) made their first contribution in [#​2388](urfave/cli#2388) - [@​fzlzjerry](https://github.com/fzlzjerry) made their first contribution in [#​2403](urfave/cli#2403) - [@​BrandonIrizarry](https://github.com/BrandonIrizarry) made their first contribution in [#​2386](urfave/cli#2386) - [@​utkarshalpha](https://github.com/utkarshalpha) made their first contribution in [#​2395](urfave/cli#2395) **Full Changelog**: <urfave/cli@v3.10.1...v3.11.0> </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At 12:00 AM through 04:59 AM and 10:00 PM through 11:59 PM, Monday through Friday (`* 0-4,22-23 * * 1-5`) - Only on Sunday and Saturday (`* * * * 0,6`) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate CLI](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0NC4xNTEuMiIsInVwZGF0ZWRJblZlciI6IjQ0LjE1MS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJLaW5kL0RlcGVuZGVuY2llcyJdfQ==--> Reviewed-on: https://altlinux.space/stapler/stplr/pulls/503
Using
text/templatedisables linker dead code elimination of exported methods for the whole program (golang/go#72895), which can noticeably increase the binary size.This adds
urfave_cli_no_templatebuild tag, which renders the default help and fish completion withouttext/template, producing the same output.Custom templates are not supported in this mode:
RootCommandHelpTemplate,CommandHelpTemplate,SubcommandHelpTemplateandFishCompletionTemplateare short string constants, only used to tell which help is requested, so modifying them is a compile error;CustomRootCommandHelpTemplateorCustomHelpTemplate), it panics. A custom template handled by a customHelpPrinteris fine.With this tag,
examples/example-cliis 3.2 MB instead of 5.1 MB, and runc is 10.5 MB instead of 12.4 MB (with opencontainers/runc#5486 applied, which fixes the only other reason DCE is disabled in runc); all 19 help pages of runc are byte-for-byte identical.Testing:
TestHelpOutput) renders various help pages and fish completion, making sure both modes produce the exact same output;TestNoTemplateTagDCEmakes sure there are no<ReflectMethod>symbols (which disable DCE) inexamples/example-clibuilt with this tag;make check-binary-sizenow also shows the size with this tag.Closes #2277.
An alternative, in which the template-less rendering code is generated from the templates, was proposed in #2442 (now closed).