diff --git a/README.md b/README.md index 44b5d16..789c6fb 100644 --- a/README.md +++ b/README.md @@ -37,11 +37,17 @@ with the filename of the changed file. (The symbol may be changed with the OPTIONS are given below: --all=false: Include normally ignored files (VCS and editor special files). + -m, --chmods=false: + Also trigger on chmod-only changes. + --clear=false: + Clear the screen before running each command. -c, --config="": A configuration file that describes how to run reflex (or '-' to read the configuration from stdin). -d, --decoration="plain": How to decorate command output. Choices: none, plain, fancy. + -D, --directory=".": + Change to this directory before watching for changes. -g, --glob=[]: A shell glob expression to match filenames. (May be repeated.) -G, --inverse-glob=[]: @@ -50,6 +56,8 @@ OPTIONS are given below: -R, --inverse-regex=[]: A regular expression to exclude matching filenames. (May be repeated.) + --name="": + Use this name instead of the numeric id in output tags. --only-dirs=false: Only match directories (not files). --only-files=false: @@ -102,8 +110,11 @@ only files that match all patterns and none of the inverse patterns are selected. The shell glob syntax is described -[here](http://golang.org/pkg/path/filepath/#Match), while the regular expression -syntax is described [here](https://code.google.com/p/re2/wiki/Syntax). +[here](https://pkg.go.dev/github.com/bmatcuk/doublestar/v4#Match), while the +regular expression syntax is described +[here](https://code.google.com/p/re2/wiki/Syntax). In addition to the standard +shell glob syntax, `**` matches any number of path segments, so `-g '**/*.go'` +matches both `main.go` and `pkg/foo/bar.go`. The path that is matched against the glob or regular expression does not have a leading `./`. For example, if there is a file `./foobar.txt` that changes, then @@ -154,6 +165,20 @@ Here's an example: -sr '\.rb$' -- \ ./bin/run_server.sh +Use `--directory` (`-D`) to run reflex from another directory. The change +happens before the configuration file is read and before watching starts, so +relative paths in the configuration and in the watched tree are resolved from +there. + +The `--decoration` (`-d`) setting may also be given on a config line. A value +on the command line takes precedence; using different values on different +lines is an error. + +If a file named `reflex.conf` exists in the working directory, reflex uses it +automatically when you run `reflex` without a config file, a command, or any +per-command flags. This means plain `reflex` picks up your config, while +`reflex -r '\.go$' make` and `reflex -c other.conf` behave as usual. + If you want to change the configuration file and have reflex reload it on the fly, you can run reflex inside reflex: @@ -211,6 +236,22 @@ use `--decoration` (`-d`) to change this output: `--decoration=none` will print the output as is; `--decoration=fancy` will color each line differently depending on which command it is, making it easier to distinguish the output. +If you give a command a name with `--name`, reflex uses that name in the tag +instead of the numeric id, which is easier to read when you run several +services. For example, a config line + + --name=web -s -- ./bin/server + +produces lines tagged `[web]`. + +### Clearing the screen + +With long-running services the output accumulates in your terminal. Pass +`--clear` to erase the screen before each command run, so you only see the +output of the latest run. When several reflexes are running, each start clears +the whole screen, including the other reflexes' output; the flag is most useful +with a single command. + ### Ignored files Reflex ignores a variety of version control and editor metadata files by diff --git a/config.go b/config.go index 6895a5e..a8dfcc0 100644 --- a/config.go +++ b/config.go @@ -20,11 +20,13 @@ type Config struct { inverseRegexes []string inverseGlobs []string subSymbol string + name string startService bool shutdownTimeout time.Duration onlyFiles bool onlyDirs bool allFiles bool + decoration string } func (c *Config) registerFlags(f *flag.FlagSet) { @@ -41,6 +43,8 @@ func (c *Config) registerFlags(f *flag.FlagSet) { f.StringVar(&c.subSymbol, "substitute", defaultSubSymbol, ` The substitution symbol that is replaced with the filename in a command.`) + f.StringVar(&c.name, "name", "", ` + Use this name instead of the numeric id in output tags.`) f.BoolVarP(&c.startService, "start-service", "s", false, ` Indicates that the command is a long-running process to be restarted on matching changes.`) @@ -54,6 +58,35 @@ func (c *Config) registerFlags(f *flag.FlagSet) { Include normally ignored files (VCS and editor special files).`) } +// registerConfigOnlyFlags registers flags that are only meaningful in a +// configuration file. Decoration is process-wide, so it is a global flag on +// the command line; allowing it per config line lets a config file set it +// once without duplicating the global flag definition. +func (c *Config) registerConfigOnlyFlags(f *flag.FlagSet) { + f.StringVarP(&c.decoration, "decoration", "d", "", ` + How to decorate command output. Choices: none, plain, fancy.`) +} + +// DecorationFromConfigs returns the decoration requested by the configs. It is +// an error for configs to ask for different decorations, since decoration +// applies to all output from the process. +func DecorationFromConfigs(configs []*Config) (string, error) { + decoration := "" + for _, c := range configs { + if c.decoration == "" { + continue + } + if decoration != "" && !strings.EqualFold(decoration, c.decoration) { + return "", fmt.Errorf("conflicting decorations in config: %q and %q", + decoration, c.decoration) + } + if decoration == "" { + decoration = c.decoration + } + } + return decoration, nil +} + // ReadConfigs reads configurations from either a file or, as a special case, // stdin if "-" is given for path. func ReadConfigs(path string) ([]*Config, error) { @@ -118,6 +151,7 @@ parseFile: flags := flag.NewFlagSet("", flag.ContinueOnError) flags.SetOutput(io.Discard) c.registerFlags(flags) + c.registerConfigOnlyFlags(flags) if err := flags.Parse(parts); err != nil { return nil, fmt.Errorf(errorf, err) } diff --git a/config_test.go b/config_test.go index 4fb37a0..db8d41f 100644 --- a/config_test.go +++ b/config_test.go @@ -15,7 +15,7 @@ func TestReadConfigs(t *testing.T) { # Some comment here -r '^a[0-9]+\.txt$' --only-dirs --substitute='[]' echo [] --g '*.go' -s --only-files echo hi +-g '*.go' -s --only-files --name=web -d fancy echo hi -r foo -r bar -R baz -g a \ -G b -G c echo "hello @@ -50,6 +50,8 @@ world" startService: true, shutdownTimeout: 500 * time.Millisecond, onlyFiles: true, + name: "web", + decoration: "fancy", }, { command: []string{"echo", "hello\nworld"}, @@ -87,3 +89,24 @@ func TestReadConfigsBad(t *testing.T) { } } } + +func TestDecorationFromConfigs(t *testing.T) { + got, err := DecorationFromConfigs([]*Config{ + {}, + {decoration: "fancy"}, + {decoration: "FANCY"}, + }) + if err != nil { + t.Fatal(err) + } + if got != "fancy" { + t.Errorf("got %q; want %q", got, "fancy") + } + + if _, err := DecorationFromConfigs([]*Config{ + {decoration: "plain"}, + {decoration: "fancy"}, + }); err == nil { + t.Error("expected an error for conflicting decorations") + } +} diff --git a/go.mod b/go.mod index c43f143..1ef69fc 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module github.com/cespare/reflex go 1.24.0 require ( + github.com/bmatcuk/doublestar/v4 v4.10.4 github.com/creack/pty v1.1.24 github.com/fsnotify/fsnotify v1.9.0 github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 diff --git a/go.sum b/go.sum index ffb3dfa..540bd89 100644 --- a/go.sum +++ b/go.sum @@ -1,3 +1,5 @@ +github.com/bmatcuk/doublestar/v4 v4.10.4 h1:oTKkRmRuOsKMqFe1AQazd9gGBFNEpOmgKp/xzB97dnA= +github.com/bmatcuk/doublestar/v4 v4.10.4/go.mod h1:xBQ8jztBU6kakFMg+8WGxn0c6z1fTSPVIjEY1Wr7jzc= github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/fsnotify/fsnotify v1.9.0 h1:2Ml+OJNzbYCTzsxtv8vKSFD9PbJjmhYF14k/jKC7S9k= diff --git a/main.go b/main.go index 3100561..86eb9fc 100644 --- a/main.go +++ b/main.go @@ -16,12 +16,17 @@ import ( const defaultSubSymbol = "{}" +const defaultConfigFile = "reflex.conf" + var ( reflexes []*Reflex flagConf string flagSequential bool flagDecoration string + flagDirectory string + flagClear bool + flagChmods bool decoration Decoration verbose bool globalFlags = flag.NewFlagSet("", flag.ContinueOnError) @@ -67,18 +72,36 @@ func init() { (or '-' to read the configuration from stdin).`) globalFlags.BoolVarP(&verbose, "verbose", "v", false, ` Verbose mode: print out more information about what reflex is doing.`) + globalFlags.BoolVarP(&flagChmods, "chmods", "m", false, ` + Also trigger on chmod-only changes.`) + globalFlags.BoolVar(&flagClear, "clear", false, ` + Clear the screen before running each command.`) globalFlags.BoolVarP(&flagSequential, "sequential", "e", false, ` Don't run multiple commands at the same time.`) globalFlags.StringVarP(&flagDecoration, "decoration", "d", "plain", ` How to decorate command output. Choices: none, plain, fancy.`) + globalFlags.StringVarP(&flagDirectory, "directory", "D", ".", ` + Change to this directory before watching for changes.`) globalConfig.registerFlags(globalFlags) } +// flagWasSet reports whether name was given on the command line. The pflag +// fork this project uses has no FlagSet.Changed, so Visit is the way to ask. +func flagWasSet(name string) bool { + set := false + globalFlags.Visit(func(f *flag.Flag) { + if f.Name == name { + set = true + } + }) + return set +} + func anyNonGlobalsRegistered() bool { any := false walkFn := func(f *flag.Flag) { switch f.Name { - case "config", "verbose", "sequential", "decoration": + case "config", "verbose", "sequential", "decoration", "directory", "chmods", "clear": default: any = true } @@ -116,6 +139,23 @@ func cleanup(reason string) { os.Exit(0) } +// resolveConfigPath decides which config file to read. An explicit --config +// always wins. Otherwise reflex uses reflex.conf automatically when it was +// invoked with no command and no per-command flags; this keeps +// `reflex -r foo make` unaffected by a reflex.conf in the directory. +func resolveConfigPath(conf string, haveCommand, perCommandFlags bool) string { + if conf != "" { + return conf + } + if haveCommand || perCommandFlags { + return "" + } + if info, err := os.Stat(defaultConfigFile); err == nil && !info.IsDir() { + return defaultConfigFile + } + return "" +} + func main() { log.SetFlags(0) if err := globalFlags.Parse(os.Args[1:]); err != nil { @@ -126,17 +166,15 @@ func main() { if verbose { printGlobals() } - switch strings.ToLower(flagDecoration) { - case "none": - decoration = DecorationNone - case "plain": - decoration = DecorationPlain - case "fancy": - decoration = DecorationFancy - default: - log.Fatalf("Invalid decoration %s. Choices: none, plain, fancy.", flagDecoration) + + if flagDirectory != "." { + if err := os.Chdir(flagDirectory); err != nil { + log.Fatalf("Could not change directory to %s: %v", flagDirectory, err) + } } + flagConf = resolveConfigPath(flagConf, globalFlags.NArg() != 0, anyNonGlobalsRegistered()) + var configs []*Config if flagConf == "" { if flagSequential { @@ -145,7 +183,7 @@ func main() { configs = []*Config{globalConfig} } else { if anyNonGlobalsRegistered() { - log.Fatal("Cannot set other flags along with --config other than --sequential, --verbose, and --decoration.") + log.Fatal("Cannot combine --config with per-command flags (such as --regex or --glob); only global flags are allowed.") } var err error configs, err = ReadConfigs(flagConf) @@ -157,6 +195,28 @@ func main() { } } + // Decoration applies to all output. A value given on the command line + // wins over one set in a config file. + if !flagWasSet("decoration") { + configDecoration, err := DecorationFromConfigs(configs) + if err != nil { + log.Fatalln(err) + } + if configDecoration != "" { + flagDecoration = configDecoration + } + } + switch strings.ToLower(flagDecoration) { + case "none": + decoration = DecorationNone + case "plain": + decoration = DecorationPlain + case "fancy": + decoration = DecorationFancy + default: + log.Fatalf("Invalid decoration %s. Choices: none, plain, fancy.", flagDecoration) + } + for _, config := range configs { reflex, err := NewReflex(config) if err != nil { diff --git a/main_test.go b/main_test.go new file mode 100644 index 0000000..5d0bb5c --- /dev/null +++ b/main_test.go @@ -0,0 +1,40 @@ +package main + +import ( + "os" + "testing" +) + +func TestResolveConfigPath(t *testing.T) { + t.Chdir(t.TempDir()) + + if got := resolveConfigPath("other.conf", true, true); got != "other.conf" { + t.Errorf("explicit config: got %q; want %q", got, "other.conf") + } + if got := resolveConfigPath("", true, false); got != "" { + t.Errorf("command without config: got %q; want no config", got) + } + if got := resolveConfigPath("", false, true); got != "" { + t.Errorf("per-command flags without config: got %q; want no config", got) + } + if got := resolveConfigPath("", false, false); got != "" { + t.Errorf("no reflex.conf: got %q; want no config", got) + } + + if err := os.WriteFile(defaultConfigFile, []byte("echo hi\n"), 0o644); err != nil { + t.Fatal(err) + } + if got := resolveConfigPath("", false, false); got != defaultConfigFile { + t.Errorf("reflex.conf present: got %q; want %q", got, defaultConfigFile) + } + + if err := os.Remove(defaultConfigFile); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(defaultConfigFile, 0o755); err != nil { + t.Fatal(err) + } + if got := resolveConfigPath("", false, false); got != "" { + t.Errorf("reflex.conf is a directory: got %q; want no config", got) + } +} diff --git a/match.go b/match.go index 1bc4b59..0f17fdf 100644 --- a/match.go +++ b/match.go @@ -2,11 +2,12 @@ package main import ( "fmt" - "path/filepath" "regexp" "regexp/syntax" "strings" "sync" + + "github.com/bmatcuk/doublestar/v4" ) // A Matcher decides whether some filename matches its set of patterns. @@ -67,7 +68,8 @@ type globMatcher struct { } func (m *globMatcher) Match(name string) bool { - matches, err := filepath.Match(m.glob, name) + // doublestar supports ** for matching across directory separators. + matches, err := doublestar.Match(m.glob, name) if err != nil { return false } diff --git a/match_test.go b/match_test.go index 5024306..017c6fb 100644 --- a/match_test.go +++ b/match_test.go @@ -49,6 +49,31 @@ func TestMatchers(t *testing.T) { } } +func TestGlobMatcherDoubleStar(t *testing.T) { + for _, tt := range []struct { + glob string + name string + want bool + }{ + {"**/*.go", "main.go", true}, + {"**/*.go", "pkg/foo/bar.go", true}, + {"**/*.go", "pkg/foo/bar.txt", false}, + {"**/*.go", "pkg/foo.go/", false}, + {"*.go", "pkg/foo.go", false}, + {"m/**", "m/a/b.go", true}, + {"foo/**/bar", "foo/bar", true}, + {"foo/**/bar", "foo/x/y/bar", true}, + {"foo/**/bar", "foo/x/y/baz", false}, + {"[", "anything", false}, + } { + m := &globMatcher{glob: tt.glob} + if got := m.Match(tt.name); got != tt.want { + t.Errorf("glob %q Match(%q): got %t; want %t", + tt.glob, tt.name, got, tt.want) + } + } +} + func TestExcludePrefix(t *testing.T) { m := newRegexMatcher(regexp.MustCompile("foo"), false) if m.ExcludePrefix("bar") { diff --git a/print.go b/print.go index a720a0b..cfc7e59 100644 --- a/print.go +++ b/print.go @@ -22,44 +22,67 @@ const ( ) type OutMsg struct { - reflexID int - msg string + reflexID int + reflexName string + msg string + clear bool } -func infoPrintln(id int, args ...interface{}) { - stdout <- OutMsg{id, strings.TrimSpace(fmt.Sprintln(args...))} +const clearScreen = "\x1b[2J\x1b[3J\x1b[H" + +func infoPrintln(id int, name string, args ...interface{}) { + stdout <- OutMsg{reflexID: id, reflexName: name, msg: strings.TrimSpace(fmt.Sprintln(args...))} } -func infoPrintf(id int, format string, args ...interface{}) { - stdout <- OutMsg{id, fmt.Sprintf(format, args...)} +func infoPrintf(id int, name string, format string, args ...interface{}) { + stdout <- OutMsg{reflexID: id, reflexName: name, msg: fmt.Sprintf(format, args...)} } func printMsg(msg OutMsg, writer io.Writer) { + var b strings.Builder + if msg.clear { + b.WriteString(clearScreen) + if msg.msg == "" { + writeLine(writer, b.String()) + return + } + } + tag := "" if decoration == DecorationFancy || decoration == DecorationPlain { - if msg.reflexID < 0 { + switch { + case msg.reflexID < 0: tag = "[info]" - } else { + case msg.reflexName != "": + tag = "[" + msg.reflexName + "]" + default: tag = fmt.Sprintf("[%02d]", msg.reflexID) } } if decoration == DecorationFancy { color := (msg.reflexID % numColors) + colorStart - if reflexID < 0 { + if msg.reflexID < 0 { color = colorRed } - fmt.Fprintf(writer, "\x1b[01;%dm%s ", color, tag) + fmt.Fprintf(&b, "\x1b[01;%dm%s ", color, tag) } else if decoration == DecorationPlain { - fmt.Fprint(writer, tag+" ") + b.WriteString(tag + " ") } - fmt.Fprint(writer, msg.msg) + b.WriteString(msg.msg) if decoration == DecorationFancy { - fmt.Fprintf(writer, "\x1b[m") + b.WriteString("\x1b[m") } if !strings.HasSuffix(msg.msg, "\n") { - fmt.Fprintln(writer) + b.WriteByte('\n') } + writeLine(writer, b.String()) +} + +// writeLine writes one complete decorated line. A terminal write error has +// nowhere to be reported, so it is intentionally dropped. +func writeLine(writer io.Writer, line string) { + _, _ = io.WriteString(writer, line) } func printOutput(out <-chan OutMsg, outWriter io.Writer) { diff --git a/reflex.go b/reflex.go index 528cca5..fb6773d 100644 --- a/reflex.go +++ b/reflex.go @@ -19,6 +19,7 @@ import ( // A Reflex is a single watch + command to execute. type Reflex struct { id int + name string source string // Describes what config/line defines this Reflex startService bool backlog Backlog @@ -27,9 +28,9 @@ type Reflex struct { onlyDirs bool command []string subSymbol string - done chan struct{} + done chan struct{} // closed when the current command exits - mu *sync.Mutex // protects killed and running + mu *sync.Mutex // protects killed, running, cmd, tty, and done killed bool running bool timeout time.Duration @@ -84,6 +85,7 @@ func NewReflex(c *Config) (*Reflex, error) { reflex := &Reflex{ id: reflexID, + name: c.name, source: c.source, startService: c.startService, backlog: backlog, @@ -92,7 +94,6 @@ func NewReflex(c *Config) (*Reflex, error) { onlyDirs: c.onlyDirs, command: c.command, subSymbol: c.subSymbol, - done: make(chan struct{}), timeout: c.shutdownTimeout, mu: &sync.Mutex{}, } @@ -105,6 +106,9 @@ func (r *Reflex) String() string { var buf bytes.Buffer fmt.Fprintln(&buf, "Reflex from", r.source) fmt.Fprintln(&buf, "| ID:", r.id) + if r.name != "" { + fmt.Fprintln(&buf, "| Name:", r.name) + } for _, matcherInfo := range strings.Split(r.matcher.String(), "\n") { fmt.Fprintln(&buf, "|", matcherInfo) } @@ -148,11 +152,11 @@ func (r *Reflex) filterMatching(out chan<- string, in <-chan string) { // batch receives file notification events and batches them up. It's a bit // tricky, but here's what it accomplishes: -// * When we initially get a message, wait a bit and batch messages before -// trying to send anything. This is because the file events come in bursts. -// * Once it's time to send, don't do it until the out channel is unblocked. -// In the meantime, keep batching. When we've sent off all the batched -// messages, go back to the beginning. +// - When we initially get a message, wait a bit and batch messages before +// trying to send anything. This is because the file events come in bursts. +// - Once it's time to send, don't do it until the out channel is unblocked. +// In the meantime, keep batching. When we've sent off all the batched +// messages, go back to the beginning. func (r *Reflex) batch(out chan<- string, in <-chan string) { const silenceInterval = 300 * time.Millisecond @@ -192,14 +196,20 @@ func (r *Reflex) runEach(names <-chan string) { for name := range names { if r.startService { if r.Running() { - infoPrintln(r.id, "Killing service") + r.infoPrintln("Killing service") r.terminate() } - infoPrintln(r.id, "Starting service") - r.runCommand(name, stdout) + r.infoPrintln("Starting service") + // A start failure is reported by runCommand; there is no process + // to wait for, so move on to the next change. + if _, err := r.runCommand(name, stdout); err != nil { + continue + } } else { - r.runCommand(name, stdout) - <-r.done + done, err := r.runCommand(name, stdout) + if err == nil { + <-done + } r.mu.Lock() r.running = false r.mu.Unlock() @@ -210,31 +220,54 @@ func (r *Reflex) runEach(names <-chan string) { func (r *Reflex) terminate() { r.mu.Lock() r.killed = true + cmd := r.cmd + tty := r.tty + done := r.done r.mu.Unlock() + + defer func() { + r.mu.Lock() + r.running = false + r.mu.Unlock() + }() + + if tty == nil && (cmd == nil || cmd.Process == nil) { + // The command never started (for example, pty.Start failed), so + // there is nothing to kill. In particular, don't wait for a done + // channel that will never be closed. See issue #105. + return + } + // Write ascii 3 (what you get from ^C) to the controlling pty. // (This won't do anything if the process already died as the write will // simply fail.) - r.tty.Write([]byte{3}) + if tty != nil { + _, _ = tty.Write([]byte{3}) + } timer := time.NewTimer(r.timeout) + defer timer.Stop() sig := syscall.SIGINT for { select { - case <-r.done: + case <-done: return case <-timer.C: + if cmd == nil || cmd.Process == nil { + return + } if sig == syscall.SIGINT { - infoPrintln(r.id, "Sending SIGINT signal...") + r.infoPrintln("Sending SIGINT signal...") } else { - infoPrintln(r.id, "Sending SIGKILL signal...") + r.infoPrintln("Sending SIGKILL signal...") } // Instead of killing the process, we want to kill its // whole pgroup in order to clean up any children the // process may have created. - if err := syscall.Kill(-1*r.cmd.Process.Pid, sig); err != nil { - infoPrintln(r.id, "Error killing:", err) - if err.(syscall.Errno) == syscall.ESRCH { // no such process + if err := syscall.Kill(-1*cmd.Process.Pid, sig); err != nil { + r.infoPrintln("Error killing:", err) + if errors.Is(err, syscall.ESRCH) { // no such process return } } @@ -257,22 +290,45 @@ func replaceSubSymbol(command []string, subSymbol string, name string) []string var seqCommands = &sync.Mutex{} // runCommand runs the given Command. All output is passed line-by-line to the -// stdout channel. -func (r *Reflex) runCommand(name string, stdout chan<- OutMsg) { +// stdout channel. It returns a channel that is closed when the command exits, +// or an error if the command could not be started. +func (r *Reflex) runCommand(name string, stdout chan<- OutMsg) (<-chan struct{}, error) { command := replaceSubSymbol(r.command, r.subSymbol, name) cmd := exec.Command(command[0], command[1:]...) - r.cmd = cmd if flagSequential { seqCommands.Lock() } + // Hold the lock across pty.Start so that terminate cannot observe a + // half-initialized command. A failed start must leave the reflex in a + // consistent "not running" state: otherwise a later terminate would + // dereference a nil Process (issue #105), and a non-service command + // would wait forever for a done signal that never comes. + r.mu.Lock() + r.killed = false + r.running = true + r.cmd = cmd tty, err := pty.Start(cmd) if err != nil { - infoPrintln(r.id, err) - return + r.cmd = nil + r.tty = nil + r.running = false + r.mu.Unlock() + if flagSequential { + seqCommands.Unlock() + } + r.infoPrintln(err) + return nil, err } + done := make(chan struct{}) r.tty = tty + r.done = done + r.mu.Unlock() + + if flagClear { + stdout <- OutMsg{clear: true} + } // Handle pty size. chResize := make(chan os.Signal, 1) @@ -290,10 +346,10 @@ func (r *Reflex) runCommand(name string, stdout chan<- OutMsg) { // Allow for lines up to 100 MB. scanner.Buffer(nil, 100e6) for scanner.Scan() { - stdout <- OutMsg{r.id, scanner.Text()} + stdout <- OutMsg{reflexID: r.id, reflexName: r.name, msg: scanner.Text()} } if err := scanner.Err(); errors.Is(err, bufio.ErrTooLong) { - infoPrintln(r.id, "Error: subprocess emitted a line longer than 100 MB") + r.infoPrintln("Error: subprocess emitted a line longer than 100 MB") } // Intentionally ignore other scanner errors. Unfortunately, // the pty returns a read error when the child dies naturally, @@ -301,15 +357,15 @@ func (r *Reflex) runCommand(name string, stdout chan<- OutMsg) { // better way to handle it. }() - r.mu.Lock() - r.running = true - r.mu.Unlock() go func() { err := cmd.Wait() if !r.Killed() && err != nil { - stdout <- OutMsg{r.id, fmt.Sprintf("(error exit: %s)", err)} + stdout <- OutMsg{reflexID: r.id, reflexName: r.name, msg: fmt.Sprintf("(error exit: %s)", err)} } - r.done <- struct{}{} + r.mu.Lock() + r.running = false + r.mu.Unlock() + close(done) signal.Stop(chResize) close(chResize) @@ -318,6 +374,8 @@ func (r *Reflex) runCommand(name string, stdout chan<- OutMsg) { seqCommands.Unlock() } }() + + return done, nil } func (r *Reflex) Start(changes <-chan string) { @@ -328,8 +386,12 @@ func (r *Reflex) Start(changes <-chan string) { go r.runEach(batched) if r.startService { // Easy hack to kick off the initial start. - infoPrintln(r.id, "Starting service") - r.runCommand("", stdout) + r.infoPrintln("Starting service") + // A start failure is reported by runCommand; there is no process to + // track, so there is nothing else to do here. + if _, err := r.runCommand("", stdout); err != nil { + return + } } } @@ -339,6 +401,10 @@ func (r *Reflex) Killed() bool { return r.killed } +func (r *Reflex) infoPrintln(args ...interface{}) { + infoPrintln(r.id, r.name, args...) +} + func (r *Reflex) Running() bool { r.mu.Lock() defer r.mu.Unlock() diff --git a/reflex_test.go b/reflex_test.go new file mode 100644 index 0000000..bf0de3f --- /dev/null +++ b/reflex_test.go @@ -0,0 +1,184 @@ +package main + +import ( + "bytes" + "os" + "sync" + "testing" + "time" +) + +// TestMain drains the stdout channel so that informational messages emitted +// during tests do not block on a full channel. +func TestMain(m *testing.M) { + go func() { + for range stdout { + } + }() + os.Exit(m.Run()) +} + +func newTestReflex(command ...string) *Reflex { + return &Reflex{ + id: 0, + command: command, + subSymbol: "{}", + timeout: 20 * time.Millisecond, + mu: &sync.Mutex{}, + } +} + +// Regression for #105: after a failed service start, running can be true while +// cmd has a nil Process. terminate must not panic and must clear running. +func TestTerminateNilSafeAfterFailedStart(t *testing.T) { + r := newTestReflex("true") + r.running = true + + done := make(chan struct{}) + go func() { + defer close(done) + r.terminate() + }() + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("terminate hung") + } + + if r.Running() { + t.Fatal("expected running=false after terminate") + } +} + +// A failed start must leave the reflex in a consistent state so that the next +// terminate does not dereference a nil Process. +func TestRunCommandFailedStartClearsRunning(t *testing.T) { + r := newTestReflex("definitely-not-a-real-reflex-binary-105") + + done, err := r.runCommand("", make(chan OutMsg, 4)) + if err == nil { + t.Fatal("expected an error for a nonexistent command") + } + if done != nil { + t.Fatal("expected no done channel after a failed start") + } + if r.Running() { + t.Fatal("expected running=false after failed start") + } + if r.Killed() { + t.Fatal("expected killed=false after failed start") + } +} + +// With --sequential, a failed start must release the global command lock, +// otherwise every subsequent command blocks forever. +func TestSequentialUnlockOnFailedStart(t *testing.T) { + oldSequential := flagSequential + flagSequential = true + defer func() { flagSequential = oldSequential }() + + r := newTestReflex("definitely-not-a-real-reflex-binary-105") + + for i := 0; i < 2; i++ { + done := make(chan struct{}) + go func() { + defer close(done) + _, _ = r.runCommand("", make(chan OutMsg, 4)) + }() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("runCommand blocked on the sequential lock after a failed start") + } + } +} + +// A non-service command that fails to start must not leave runEach waiting on +// a done signal that will never arrive. +func TestRunEachDoesNotHangAfterFailedStart(t *testing.T) { + r := newTestReflex("definitely-not-a-real-reflex-binary-105") + + names := make(chan string, 2) + names <- "a" + names <- "b" + close(names) + + finished := make(chan struct{}) + go func() { + defer close(finished) + r.runEach(names) + }() + + select { + case <-finished: + case <-time.After(3 * time.Second): + t.Fatal("runEach hung after a failed start") + } +} + +// killed is per-process: terminating a service must not suppress error output +// from later runs of the same reflex. + +func TestKilledResetsBetweenRuns(t *testing.T) { + r := newTestReflex("sh", "-c", "sleep 5") + + if _, err := r.runCommand("", make(chan OutMsg, 16)); err != nil { + t.Fatalf("runCommand: %v", err) + } + + terminateDone := make(chan struct{}) + go func() { + defer close(terminateDone) + r.terminate() + }() + + select { + case <-terminateDone: + case <-time.After(3 * time.Second): + t.Fatal("terminate hung") + } + if !r.Killed() { + t.Fatal("expected killed=true after terminate") + } + if r.Running() { + t.Fatal("expected running=false after terminate") + } + + // A new run must reset killed so that failures are reported again. + r.command = []string{"sh", "-c", "exit 0"} + done, err := r.runCommand("", make(chan OutMsg, 16)) + if err != nil { + t.Fatalf("second runCommand: %v", err) + } + <-done + if r.Killed() { + t.Fatal("expected killed=false at the start of a new run") + } + if r.Running() { + t.Fatal("expected running=false after the command exited") + } +} + +func TestPrintMsgTags(t *testing.T) { + oldDecoration := decoration + decoration = DecorationPlain + defer func() { decoration = oldDecoration }() + + for _, tt := range []struct { + name string + msg OutMsg + want string + }{ + {"numeric", OutMsg{reflexID: 3, msg: "hello"}, "[03] hello\n"}, + {"named", OutMsg{reflexID: 3, reflexName: "web", msg: "hello"}, "[web] hello\n"}, + {"info", OutMsg{reflexID: -1, msg: "oops"}, "[info] oops\n"}, + {"clear", OutMsg{clear: true}, clearScreen}, + } { + var b bytes.Buffer + printMsg(tt.msg, &b) + if got := b.String(); got != tt.want { + t.Errorf("%s: got %q; want %q", tt.name, got, tt.want) + } + } +} diff --git a/watch.go b/watch.go index f591a87..4171120 100644 --- a/watch.go +++ b/watch.go @@ -16,27 +16,27 @@ const chmodMask fsnotify.Op = ^fsnotify.Op(0) ^ fsnotify.Chmod // criteria of all reflexes can be ignored. func watch(root string, watcher *fsnotify.Watcher, names chan<- string, done chan<- error, reflexes []*Reflex) { if err := filepath.Walk(root, walker(watcher, reflexes)); err != nil { - infoPrintf(-1, "Error while walking path %s: %s", root, err) + infoPrintf(-1, "", "Error while walking path %s: %s", root, err) } for { select { case e := <-watcher.Events: if verbose { - infoPrintln(-1, "fsnotify event:", e) + infoPrintln(-1, "", "fsnotify event:", e) } stat, err := os.Stat(e.Name) if err != nil { continue } path := normalize(e.Name, stat.IsDir()) - if e.Op&chmodMask == 0 { + if !flagChmods && e.Op&chmodMask == 0 { continue } names <- path if e.Op&fsnotify.Create > 0 && stat.IsDir() { if err := filepath.Walk(path, walker(watcher, reflexes)); err != nil { - infoPrintf(-1, "Error while walking path %s: %s", path, err) + infoPrintf(-1, "", "Error while walking path %s: %s", path, err) } } // TODO: Cannot currently remove fsnotify watches @@ -68,7 +68,7 @@ func walker(watcher *fsnotify.Watcher, reflexes []*Reflex) filepath.WalkFunc { return filepath.SkipDir } if err := watcher.Add(path); err != nil { - infoPrintf(-1, "Error while watching new path %s: %s", path, err) + infoPrintf(-1, "", "Error while watching new path %s: %s", path, err) } return nil }