Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmd/cue: cue help does too much work #2161

Closed
rogpeppe opened this issue Nov 30, 2022 · 1 comment
Closed

cmd/cue: cue help does too much work #2161

rogpeppe opened this issue Nov 30, 2022 · 1 comment
Labels
NeedsFix

Comments

@rogpeppe
Copy link
Member

What version of CUE are you using (cue version)?

$ cue version
cue version v0.0.0-20221128094749-4fee020034c6

      -buildmode exe
       -compiler gc
     CGO_ENABLED 1
          GOARCH amd64
            GOOS linux
         GOAMD64 v1
             vcs git
    vcs.revision 4fee020034c66d74c2a0d68d56c72f7728bd8ea3
        vcs.time 2022-11-28T09:47:49Z
    vcs.modified false

Does this issue reproduce with the latest release?

Yes

What did you do?

Create the following testscript, say slowhelp.txtar:

exec sh -c 'echo package foo > x.cue; yes "x: {a: 1, b: 2, c: \"hello\"}" | head -10000 >> x.cue'
exec cue help

Run it as:

time testscript slowhelp.txtar

What did you expect to see?

A fairly quick execution time.

What did you see instead?

2.28user 0.08system 0:02.18elapsed 108%CPU (0avgtext+0avgdata 62404maxresident)k
15400inputs+560outputs (60major+19078minor)pagefaults 0swaps

It took over 2s to run cue help. If there's no cue file in the directory where cue help is running,
the runtime is under 100ms.

In general, invoking cue help should not depend on the content of the current directory.
@rogpeppe rogpeppe added NeedsInvestigation Triage Requires triage/attention NeedsFix and removed NeedsInvestigation Triage Requires triage/attention labels Nov 30, 2022
@mvdan
Copy link
Member

mvdan commented Mar 26, 2023

I just realised that this is on purpose, because cue cmd foo also works as cue foo, so cue help cmd is trying to be... helpful by first loading the current package to find all defined CUE commands, so that they can be added to the help output.

For example, emphasis mine:

$ cd internal/ci
$ cue help cmd
[...]
Usage:
  cue cmd <name> [inputs] [flags]
  cue cmd [command]

Available Commands:
  gen         gen.workflows regenerates the GitHub workflow Yaml definitions.
[...]

However, I don't think this extra work affects commands like cue -h or cue help, so we could probably narrow it down.

@mvdan mvdan mentioned this issue Mar 26, 2023
Open
@cueckoo cueckoo closed this as completed in 143b102 Apr 5, 2023
cueckoo pushed a commit that referenced this issue Apr 18, 2023
@mvdan
cmd/cue: refactor our use of cobra
59080b6 
Our API in cmd/cue/cmd was somewhat confused;
the New function was only meant to create the root command,
but it also returned an error as it had to do more work upfront.
In particular, it tried to interpret arguments and flags to support:

1) `cue mycmd` being a shortcut to `cue cmd mycmd`

2) `cue help cmd` and `cue help cmd mycmd` loading custom command info

This code was messy and has caused bugs in the past,
such as `cue help` loading the current package unnecessarily (#2161),
or TestHelp being chatty since New could print usage text
before cobra.Command.SetOutput could be called (#1694).

Instead, make New do very little work, and offload the two cases above:

1) Allow the root command to take arbitrary arguments,
   meaning that its RunE func is used as a fallback when the first
   argument does not match any built-in sub-command like `fix`.
   Said RunE func is simply forwarded to `cue cmd`,
   which is taught how to load `*_tool.cue` files just like New used to,
   to then run the specified command.

2) Add our own custom "help" command, a version of cobra's default
   which knows how to load `*_tool.cue` files and add all of their
   custom commands when using `cue help cmd [args]`.

For consistency with "help" now being our own command and not cobra's,
make the "-h" or "--help" flag our own, and hide it from the usage text.

We also slightly improve the output from `cue foo` and `cue cmd foo`
when "foo" is not a known built-in nor custom command,
which can be seen from the changes in testdata.

While here, update the version of cobra as well.

Finally, add copious amounts of documentation to the tricky bits,
because it took some digging to figure out how to make it work well.

Note that a bit more cleanup can be done in the other subcommands,
but this CL focuses on "cmd" and "help" as they were the biggest ones.

Also note that we don't change the API for New and Command.Run,
although New always returns a nil error and args are only used by Run,
because it feels like unnecessary churn to break the API right now.
Plus, this refactor is already large enough as it is.

Fixes #1694.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Change-Id: Iabe6e5724e2da9afbab8897fa22b49883e195733
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/552733
Unity-Result: CUEcueckoo <cueckoo@cuelang.org>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Reviewed-by: Roger Peppe <rogpeppe@gmail.com>
@mvdan mvdan mentioned this issue Aug 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@rogpeppe @mvdan