From 02a63d0e25a89bb31e4702ce2a92f827fcaab350 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gabriel=20N=C3=BCtzi?= Date: Sun, 13 Nov 2022 08:37:29 +0100 Subject: [PATCH] fix: Correct installer log problems and its wrong error handling - Installer is guarded to only run once. - Installer log should now be visible when installer crashes. --- githooks/apps/cli/cli.go | 41 +++++++----- githooks/apps/cli/cli_test.go | 7 +- githooks/cmd/installer/installer.go | 87 ++++++++++++++++++------- githooks/cmd/uninstaller/uninstaller.go | 4 +- githooks/common/error-handler.go | 3 +- githooks/common/signals.go | 13 +++- tests/exec-rules.sh | 2 +- tests/exec-steps.sh | 4 +- tests/exec-tests.sh | 1 + tests/exec-testsuite.sh | 2 +- tests/test-alpine-nolfs.sh | 3 + tests/test-alpine-user.sh | 3 + tests/test-alpine.sh | 3 + tests/test-corehookspath.sh | 4 ++ tests/test-coverage.sh | 8 ++- tests/test-rules.sh | 8 ++- tests/test-testsuite.sh | 5 ++ tests/test-whitespace.sh | 4 ++ tests/test-windows.sh | 5 ++ 19 files changed, 153 insertions(+), 54 deletions(-) diff --git a/githooks/apps/cli/cli.go b/githooks/apps/cli/cli.go index 5705a09d..712baba2 100644 --- a/githooks/apps/cli/cli.go +++ b/githooks/apps/cli/cli.go @@ -10,7 +10,25 @@ import ( "github.com/gabyx/githooks/githooks/hooks" ) -func mainRun() (exitCode int) { +func installSignalHandling() *cm.InterruptContext { + var cleanUpX cm.InterruptContext + + c := make(chan os.Signal, 1) + signal.Notify(c, os.Interrupt) + defer func() { + signal.Stop(c) + }() + + go func() { + <-c + cleanUpX.RunHandlers() + os.Exit(1) // Return 1 := canceled always... + }() + + return &cleanUpX +} + +func mainRun(cleanUpX *cm.InterruptContext) (exitCode int) { log, err := cm.CreateLogContext(false) cm.AssertOrPanic(err == nil, "Could not create log") @@ -29,21 +47,7 @@ func mainRun() (exitCode int) { } }() - // Install signal handling - var cleanUpX cm.InterruptContext - c := make(chan os.Signal, 1) - signal.Notify(c, os.Interrupt) - defer func() { - signal.Stop(c) - }() - - go func() { - <-c - cleanUpX.RunHandlers() - os.Exit(1) // Return 1 := canceled always... - }() - - err = cmd.Run(log, log, wrapPanicExitCode, &cleanUpX) + err = cmd.Run(log, log, wrapPanicExitCode, cleanUpX) // Overwrite the exit code if its a command exit error. if v, ok := err.(ccm.CmdExit); ok { @@ -56,5 +60,8 @@ func mainRun() (exitCode int) { } func main() { - os.Exit(mainRun()) + cleanUpX := installSignalHandling() + exitCode := mainRun(cleanUpX) + cleanUpX.RunHandlers() + os.Exit(exitCode) } diff --git a/githooks/apps/cli/cli_test.go b/githooks/apps/cli/cli_test.go index 9d7108de..2fdd3ed4 100644 --- a/githooks/apps/cli/cli_test.go +++ b/githooks/apps/cli/cli_test.go @@ -4,6 +4,7 @@ package main import ( "github.com/gabyx/githooks/githooks/apps/coverage" + cm "github.com/gabyx/githooks/githooks/common" "testing" ) @@ -14,7 +15,11 @@ func TestCoverage(t *testing.T) { // fmt.Printf("Forward args: %q\n", os.Args) // Run the main binary... - if mainRun() != 0 { + var cleanUpX cm.InterruptContext + exitCode := mainRun(&cleanUpX) + cleanUpX.RunHandlers() + + if exitCode != 0 { t.Fatal() } } diff --git a/githooks/cmd/installer/installer.go b/githooks/cmd/installer/installer.go index 8e6c235b..57a01b66 100644 --- a/githooks/cmd/installer/installer.go +++ b/githooks/cmd/installer/installer.go @@ -36,8 +36,8 @@ func NewCmd(ctx *ccm.CmdContext) *cobra.Command { Long: "Githooks installer application\n" + "See further information at https://github.com/gabyx/githooks/blob/main/README.md", PreRun: ccm.PanicIfAnyArgs(ctx.Log), - Run: func(cmd *cobra.Command, _ []string) { - runInstall(cmd, ctx, vi) + RunE: func(cmd *cobra.Command, _ []string) error { + return runInstall(cmd, ctx, vi) }} defineArguments(cmd, vi) @@ -375,16 +375,18 @@ func getDeploySettings( return deploySettings } -func prepareDispatch( +func runInstallDispatched( log cm.ILogContext, gitx *git.Context, settings *Settings, - args *Arguments, + args Arguments, cleanUpX *cm.InterruptContext) (bool, error) { var status updates.ReleaseStatus var err error + log.Info("Running dispatched installer.") + if args.InternalAutoUpdate { log.Info("Executing auto update...") @@ -411,9 +413,7 @@ func prepareDispatch( log.DebugF("Status: %v", status) } - updateAvailable := status.LocalCommitSHA != status.RemoteCommitSHA - - cm.PanicIfF(args.InternalAutoUpdate && !updateAvailable, + cm.PanicIfF(args.InternalAutoUpdate && !status.IsUpdateAvailable, "An autoupdate should only be triggered when and update is found.") installer := hooks.GetInstallerExecutable(settings.InstallDir) @@ -423,9 +423,12 @@ func prepareDispatch( // or the installer is missing. binaries := updates.Binaries{} - if updateAvailable || !haveInstaller { + log.InfoF("Githooks update available: '%v'", status.IsUpdateAvailable) + log.InfoF("Githooks installer existing: '%v'", haveInstaller) + + if status.IsUpdateAvailable || !haveInstaller { - log.Info("Getting Githooks binaries...") + log.Info("Getting Githooks binaries ...") tempDir, err := os.MkdirTemp(os.TempDir(), "githooks-update-*") log.AssertNoErrorPanic(err, "Can not create temporary update dir in '%s'", os.TempDir()) @@ -459,7 +462,7 @@ func prepareDispatch( log.InfoF("Download '%s' from deploy source...", tag) - deploySettings := getDeploySettings(log, settings.InstallDir, status.RemoteURL, args) + deploySettings := getDeploySettings(log, settings.InstallDir, status.RemoteURL, &args) binaries = downloadBinaries(log, deploySettings, tempDir, tag) } @@ -467,9 +470,10 @@ func prepareDispatch( } // Set variables for further update procedure... + // Note: `args` is passed by value. args.InternalPostDispatch = true args.InternalBinaries = binaries.All - if updateAvailable { + if status.IsUpdateAvailable { args.InternalUpdateFromVersion = build.BuildVersion args.InternalUpdateTo = status.UpdateCommitSHA } @@ -480,7 +484,7 @@ func prepareDispatch( log.PanicIfF(!cm.IsFile(installer.Cmd), "Githooks executable '%s' is not existing.", installer) - return true, runInstaller(log, &installer, args) + return true, runInstaller(log, &installer, &args) } func runInstaller(log cm.ILogContext, installer cm.IExecutable, args *Arguments) error { @@ -1287,28 +1291,50 @@ func runUpdate( } } -func addInstallerLog(path string, log cm.ILogContext) string { +func addInstallerLog(path string, log cm.ILogContext) (isDefault bool, resPath string) { var err error var file *os.File if strs.IsEmpty(path) { file, err = os.CreateTemp("", "githooks-installer-*.log") log.AssertNoErrorF(err, "Failed to create installer log at '%v'", path) + isDefault = true } else { file, err = os.OpenFile(path, os.O_CREATE|os.O_RDWR|os.O_APPEND, cm.DefaultFileModeFile) log.AssertNoErrorF(err, "Failed to append to installer log at '%v'", path) } if err != nil { - return "" + return } log.AddFileWriter(file) + resPath = file.Name() - return file.Name() + return +} + +func assertOneInstallerRunning(log cm.ILogContext, interruptCtx *cm.InterruptContext) { + lockFile := path.Join(os.TempDir(), "githooks-installer-lock") + if exists, _ := cm.IsPathExisting(lockFile); exists { + log.PanicF("Only one Githooks installer can run at the same time. "+ + "Maybe delete the lock file '%v", lockFile) + } + + log.DebugF("Created lockfile '%s'.", lockFile) + err := cm.TouchFile(lockFile, true) + log.AssertNoErrorPanic(err, "Could not create lockfile '%v'.", lockFile) + + // Remove the lock on any exit. + deleteLock := func() { + log.DebugF("Remove lockfile '%s'.", lockFile) + err := os.Remove(lockFile) + log.AssertNoError(err, "Lockfile not removed?") + } + interruptCtx.AddHandler(deleteLock) } -func runInstall(cmd *cobra.Command, ctx *ccm.CmdContext, vi *viper.Viper) { +func runInstall(cmd *cobra.Command, ctx *ccm.CmdContext, vi *viper.Viper) error { args := Arguments{} log := ctx.Log @@ -1317,7 +1343,9 @@ func runInstall(cmd *cobra.Command, ctx *ccm.CmdContext, vi *viper.Viper) { initArgs(log, &args, vi) validateArgs(log, cmd, &args) - args.Log = addInstallerLog(args.Log, log) + isDefaultLog := false + isDefaultLog, args.Log = addInstallerLog(args.Log, log) + log.InfoF("Githooks Installer [version: %s]", build.BuildVersion) dt := time.Now() log.InfoF("Started at: %s", dt.String()) @@ -1329,16 +1357,16 @@ func runInstall(cmd *cobra.Command, ctx *ccm.CmdContext, vi *viper.Viper) { if r := recover(); r != nil { panic(r) } - log.RemoveFileWriter() - if RemoveInstallerLogOnSuccess && !args.InternalPostDispatch && - logStats.ErrorCount() == 0 { + + if RemoveInstallerLogOnSuccess && logStats.ErrorCount() == 0 && + isDefaultLog && !args.InternalPostDispatch { + log.RemoveFileWriter() _ = os.Remove(args.Log) } }() } - log.InfoF("Logfile: '%s'", args.Log) - + log.InfoF("Log file: '%s'", args.Log) settings, uiSettings := setupSettings(log, ctx.GitX, &args) log.DebugF("Arguments: %+v", args) @@ -1349,11 +1377,16 @@ func runInstall(cmd *cobra.Command, ctx *ccm.CmdContext, vi *viper.Viper) { } if !args.InternalPostDispatch { - isDispatched, err := prepareDispatch(log, ctx.GitX, &settings, &args, ctx.CleanupX) + assertOneInstallerRunning(log, ctx.CleanupX) + + isDispatched, err := runInstallDispatched(log, ctx.GitX, &settings, args, ctx.CleanupX) log.MoveFileWriterToEnd() // We are logging to the same file. Move it to the end. - log.AssertNoErrorPanic(err, "Running installer failed.") + if err != nil { + return ctx.NewCmdExit(1, "%v", err) + } + if isDispatched { - return + return nil } // intended fallthrough ... (only debug) } @@ -1367,7 +1400,11 @@ func runInstall(cmd *cobra.Command, ctx *ccm.CmdContext, vi *viper.Viper) { " • %v errors\n"+ " • %v warnings\n"+ "occurred!", logStats.ErrorCount(), logStats.WarningCount()) + + return ctx.NewCmdExit(1, "Install failed.") } + + return nil } func transformLegacyGitConfigSettings(log cm.ILogContext, gitx *git.Context) { diff --git a/githooks/cmd/uninstaller/uninstaller.go b/githooks/cmd/uninstaller/uninstaller.go index 204980cf..bbd84aae 100644 --- a/githooks/cmd/uninstaller/uninstaller.go +++ b/githooks/cmd/uninstaller/uninstaller.go @@ -113,7 +113,7 @@ func setupSettings(log cm.ILogContext, gitx *git.Context, args *Arguments) (Sett UISettings{PromptCtx: promptx} } -func prepareDispatch(log cm.ILogContext, settings *Settings, args *Arguments) bool { +func runDispatchedInstall(log cm.ILogContext, settings *Settings, args *Arguments) bool { uninstaller := hooks.GetUninstallerExecutable(settings.InstallDir) if !cm.IsFile(uninstaller.Cmd) { @@ -384,7 +384,7 @@ func runUninstall(ctx *ccm.CmdContext, vi *viper.Viper) { settings, uiSettings := setupSettings(log, ctx.GitX, &args) if !args.InternalPostDispatch { - if isDispatched := prepareDispatch(log, &settings, &args); isDispatched { + if isDispatched := runDispatchedInstall(log, &settings, &args); isDispatched { return } } diff --git a/githooks/common/error-handler.go b/githooks/common/error-handler.go index 7cea39e4..01804e84 100644 --- a/githooks/common/error-handler.go +++ b/githooks/common/error-handler.go @@ -9,7 +9,8 @@ import ( func HandleCLIErrors( err interface{}, log ILogContext, - getBugReportingInfo func() string) bool { + getBugReportingInfo func() string, +) bool { if err == nil { return false diff --git a/githooks/common/signals.go b/githooks/common/signals.go index f6d0c11b..e1c969e6 100644 --- a/githooks/common/signals.go +++ b/githooks/common/signals.go @@ -1,9 +1,12 @@ package common -// Cleanup context with registered handlers which can be executed in reverse order. -// Mainly for signal intertupts. +// Cleanup context with registered handlers which can be executed in reverse order +// only once. +// Mainly for signal interrupts. type InterruptContext struct { handlers []func() + + isRun bool } func (c *InterruptContext) AddHandler(handler func()) { @@ -11,7 +14,13 @@ func (c *InterruptContext) AddHandler(handler func()) { } func (c *InterruptContext) RunHandlers() { + if c.isRun { + return + } + for i := range c.handlers { c.handlers[len(c.handlers)-i-1]() } + + c.isRun = true } diff --git a/tests/exec-rules.sh b/tests/exec-rules.sh index 9a3d8c6e..3979ed30 100755 --- a/tests/exec-rules.sh +++ b/tests/exec-rules.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash -if ! grep '/docker/' /dev/null 2>&1; then +if [ "$DOCKER_RUNNING" != "true" ]; then echo "! This script is only meant to be run in a Docker container" exit 1 fi diff --git a/tests/exec-steps.sh b/tests/exec-steps.sh index 7873c7b0..185de370 100755 --- a/tests/exec-steps.sh +++ b/tests/exec-steps.sh @@ -3,8 +3,8 @@ if [ "$1" = "--skip-docker-check" ]; then shift else - if ! grep '/docker/' /dev/null 2>&1; then - echo "! This script is only meant to be run in a Docker container" >&2 + if [ "$DOCKER_RUNNING" != "true" ]; then + echo "! This script is only meant to be run in a Docker container" exit 1 fi fi diff --git a/tests/exec-tests.sh b/tests/exec-tests.sh index 1a2ec323..9dbe2647 100755 --- a/tests/exec-tests.sh +++ b/tests/exec-tests.sh @@ -13,6 +13,7 @@ fi cat </dev/null 2>&1; then +if [ "$DOCKER_RUNNING" != "true" ]; then echo "! This script is only meant to be run in a Docker container" exit 1 fi diff --git a/tests/test-alpine-nolfs.sh b/tests/test-alpine-nolfs.sh index 7d35a36c..9643b827 100755 --- a/tests/test-alpine-nolfs.sh +++ b/tests/test-alpine-nolfs.sh @@ -6,6 +6,9 @@ cat <