From 8c21b84d5a88d42482b6c222ec1be59f2a063e1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Wed, 16 Oct 2024 16:35:36 +0100 Subject: [PATCH] cmd/cue/cmd: undo panic when multiple commands are run in one process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This should resolve the panics encountered by cmd/cue/cmd Go API users who were running multiple commands in a single Go process, which is a fairly reasonable use case which worked before. This mostly reverts https://cuelang.org/cl/1198496, but it adds a note for future reference to the relevant code and updates the new test. I briefly considered alternative routes such as only doing the mkRunE setup work on the first command run. However, it's not clear to me that such a strategy would lead to better behavior for those users. For example, if they rely on the collection of CUE stats, it would be very odd if only the first command run were to produce them. Fixes #3458. Signed-off-by: Daniel Martí Change-Id: Iadb274de8c3d233796ee6feb33e40ffc03bc0f08 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1202646 Reviewed-by: Roger Peppe TryBot-Result: CUEcueckoo Unity-Result: CUE porcuepine Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1202871 --- cmd/cue/cmd/root.go | 16 ++++++---------- cmd/cue/cmd/root_test.go | 9 +++------ 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/cmd/cue/cmd/root.go b/cmd/cue/cmd/root.go index 11bea7b1723..dea05c9129c 100644 --- a/cmd/cue/cmd/root.go +++ b/cmd/cue/cmd/root.go @@ -85,20 +85,16 @@ type Stats struct { } } -var hasRunCommand bool - func mkRunE(c *Command, f runFunction) func(*cobra.Command, []string) error { return func(cmd *cobra.Command, args []string) error { - // The init work below should only happen once per cmd/cue invocation; - // if it happens twice, we'll misbehave by writing stats twice - // or miscalculating pprof and stats numbers. - if hasRunCommand { - panic("cmd/cue/cmd.mkRunE init ran twice") - } - hasRunCommand = true - c.Command = cmd + // Note that the setup code below should only run once per cmd/cue invocation. + // This is because part of it modifies the global state like cueexperiment, + // but also because running this twice may result in broken CUE stats or Go profiles. + // However, users of the exposed Go API may be creating and running many commands, + // so we can't panic or fail if this setup work happens twice. + statsEnc, err := statsEncoder(c) if err != nil { return err diff --git a/cmd/cue/cmd/root_test.go b/cmd/cue/cmd/root_test.go index 648e55a82ce..30737858911 100644 --- a/cmd/cue/cmd/root_test.go +++ b/cmd/cue/cmd/root_test.go @@ -56,10 +56,7 @@ func TestCommand(t *testing.T) { qt.Assert(t, qt.Equals(buf.String(), "{\n \"foo\": 123\n}\n")) // Verify that we can use the API exposed by the embedded cobra command. - // TODO(mvdan): panics due to https://cuelang.org/issue/3458; fix it. - qt.Assert(t, qt.PanicMatches(func() { - c, err = cmd.New([]string{"fmt", "nosuchfile.cue"}) - err = c.Execute() - qt.Assert(t, qt.IsNotNil(err)) - }, "cmd/cue/cmd.mkRunE init ran twice")) + c, err = cmd.New([]string{"fmt", "nosuchfile.cue"}) + err = c.Execute() + qt.Assert(t, qt.IsNotNil(err)) }