From c0b649d5882db6e93a6aa9c74dfdc12eaa994baf Mon Sep 17 00:00:00 2001 From: ThinkChaos Date: Mon, 4 Mar 2024 20:10:40 -0500 Subject: [PATCH 1/5] refactor: partially split main function --- cmd/gonic/gonic.go | 140 +++++++++++++++++++++++++++------------------ 1 file changed, 85 insertions(+), 55 deletions(-) diff --git a/cmd/gonic/gonic.go b/cmd/gonic/gonic.go index 3235e1d78..a48ce6853 100644 --- a/cmd/gonic/gonic.go +++ b/cmd/gonic/gonic.go @@ -49,49 +49,62 @@ import ( "go.senan.xyz/gonic/transcode" ) -func main() { - confListenAddr := flag.String("listen-addr", "0.0.0.0:4747", "listen address (optional)") +//nolint:gochecknoglobals +var ( + confListenAddr = flag.String("listen-addr", "0.0.0.0:4747", "listen address (optional)") + + confTLSCert = flag.String("tls-cert", "", "path to TLS certificate (optional)") + confTLSKey = flag.String("tls-key", "", "path to TLS private key (optional)") - confTLSCert := flag.String("tls-cert", "", "path to TLS certificate (optional)") - confTLSKey := flag.String("tls-key", "", "path to TLS private key (optional)") + confPodcastPurgeAgeDays = flag.Uint("podcast-purge-age", 0, "age (in days) to purge podcast episodes if not accessed (optional)") + confPodcastPath = flag.String("podcast-path", "", "path to podcasts") - confPodcastPurgeAgeDays := flag.Uint("podcast-purge-age", 0, "age (in days) to purge podcast episodes if not accessed (optional)") - confPodcastPath := flag.String("podcast-path", "", "path to podcasts") + confCachePath = flag.String("cache-path", "", "path to cache") - confCachePath := flag.String("cache-path", "", "path to cache") + confMusicPaths = flagVar[pathAliases]("music-path", "path to music") - var confMusicPaths pathAliases - flag.Var(&confMusicPaths, "music-path", "path to music") + confPlaylistsPath = flag.String("playlists-path", "", "path to your list of new or existing m3u playlists that gonic can manage") - confPlaylistsPath := flag.String("playlists-path", "", "path to your list of new or existing m3u playlists that gonic can manage") + confDBPath = flag.String("db-path", "gonic.db", "path to database (optional)") - confDBPath := flag.String("db-path", "gonic.db", "path to database (optional)") + confScanIntervalMins = flag.Uint("scan-interval", 0, "interval (in minutes) to automatically scan music (optional)") + confScanAtStart = flag.Bool("scan-at-start-enabled", false, "whether to perform an initial scan at startup (optional)") + confScanWatcher = flag.Bool("scan-watcher-enabled", false, "whether to watch file system for new music and rescan (optional)") - confScanIntervalMins := flag.Uint("scan-interval", 0, "interval (in minutes) to automatically scan music (optional)") - confScanAtStart := flag.Bool("scan-at-start-enabled", false, "whether to perform an initial scan at startup (optional)") - confScanWatcher := flag.Bool("scan-watcher-enabled", false, "whether to watch file system for new music and rescan (optional)") + confJukeboxEnabled = flag.Bool("jukebox-enabled", false, "whether the subsonic jukebox api should be enabled (optional)") + confJukeboxMPVExtraArgs = flag.String("jukebox-mpv-extra-args", "", "extra command line arguments to pass to the jukebox mpv daemon (optional)") - confJukeboxEnabled := flag.Bool("jukebox-enabled", false, "whether the subsonic jukebox api should be enabled (optional)") - confJukeboxMPVExtraArgs := flag.String("jukebox-mpv-extra-args", "", "extra command line arguments to pass to the jukebox mpv daemon (optional)") + confProxyPrefix = flag.String("proxy-prefix", "", "url path prefix to use if behind proxy. eg '/gonic' (optional)") + confHTTPLog = flag.Bool("http-log", true, "http request logging (optional)") - confProxyPrefix := flag.String("proxy-prefix", "", "url path prefix to use if behind proxy. eg '/gonic' (optional)") - confHTTPLog := flag.Bool("http-log", true, "http request logging (optional)") + confShowVersion = flag.Bool("version", false, "show gonic version") + confConfigPath = flag.String("config-path", "", "path to config (optional)") - confShowVersion := flag.Bool("version", false, "show gonic version") - confConfigPath := flag.String("config-path", "", "path to config (optional)") + confExcludePattern = flag.String("exclude-pattern", "", "regex pattern to exclude files from scan (optional)") - confExcludePattern := flag.String("exclude-pattern", "", "regex pattern to exclude files from scan (optional)") + confMultiValueGenre = flagVar[multiValueSetting]("multi-value-genre", "setting for mutli-valued genre scanning (optional)") + confMultiValueArtist = flagVar[multiValueSetting]("multi-value-artist", "setting for mutli-valued track artist scanning (optional)") + confMultiValueAlbumArtist = flagVar[multiValueSetting]("multi-value-album-artist", "setting for mutli-valued album artist scanning (optional)") - var confMultiValueGenre, confMultiValueArtist, confMultiValueAlbumArtist multiValueSetting - flag.Var(&confMultiValueGenre, "multi-value-genre", "setting for mutli-valued genre scanning (optional)") - flag.Var(&confMultiValueArtist, "multi-value-artist", "setting for mutli-valued track artist scanning (optional)") - flag.Var(&confMultiValueAlbumArtist, "multi-value-album-artist", "setting for mutli-valued album artist scanning (optional)") + confPprof = flag.Bool("pprof", false, "enable the /debug/pprof endpoint (optional)") + confExpvar = flag.Bool("expvar", false, "enable the /debug/vars endpoint (optional)") - confPprof := flag.Bool("pprof", false, "enable the /debug/pprof endpoint (optional)") - confExpvar := flag.Bool("expvar", false, "enable the /debug/vars endpoint (optional)") + deprecatedConfGenreSplit = flag.String("genre-split", "", "(deprecated, see multi-value settings)") +) + +type flagValue[T any] interface { + flag.Value + *T +} - deprecatedConfGenreSplit := flag.String("genre-split", "", "(deprecated, see multi-value settings)") +func flagVar[T any, TPtr flagValue[T]](name, usage string) *T { + storage := new(T) + flag.Var((TPtr)(storage), name, usage) + return storage +} + +func loadConfig() error { flag.Parse() flagconf.ParseEnv() flagconf.ParseConfig(*confConfigPath) @@ -102,58 +115,75 @@ func main() { } if _, err := regexp.Compile(*confExcludePattern); err != nil { - log.Fatalf("invalid exclude pattern: %v\n", err) + return fmt.Errorf("invalid exclude pattern: %w", err) } - if len(confMusicPaths) == 0 { - log.Fatalf("please provide a music directory") + if len(*confMusicPaths) == 0 { + return errors.New("please provide a music directory") } var err error - for i, confMusicPath := range confMusicPaths { - if confMusicPaths[i].path, err = validatePath(confMusicPath.path); err != nil { - log.Fatalf("checking music dir %q: %v", confMusicPath.path, err) + for i, confMusicPath := range *confMusicPaths { + if (*confMusicPaths)[i].path, err = validatePath(confMusicPath.path); err != nil { + return fmt.Errorf("checking music dir %q: %w", confMusicPath.path, err) } } - if *confPodcastPath, err = validatePath(*confPodcastPath); err != nil { - log.Fatalf("checking podcast directory: %v", err) + if *confPodcastPath, err = validatePath(*confPodcastPath); err != nil && *confPodcastPath != "" { + return fmt.Errorf("checking podcast directory: %w", err) } if *confCachePath, err = validatePath(*confCachePath); err != nil { - log.Fatalf("checking cache directory: %v", err) + return fmt.Errorf("checking cache directory: %w", err) } if *confPlaylistsPath, err = validatePath(*confPlaylistsPath); err != nil { - log.Fatalf("checking playlist directory: %v", err) + return fmt.Errorf("checking playlist directory: %w", err) } - cacheDirAudio := path.Join(*confCachePath, "audio") - cacheDirCovers := path.Join(*confCachePath, "covers") - if err := os.MkdirAll(cacheDirAudio, os.ModePerm); err != nil { - log.Fatalf("couldn't create audio cache path: %v\n", err) - } - if err := os.MkdirAll(cacheDirCovers, os.ModePerm); err != nil { - log.Fatalf("couldn't create covers cache path: %v\n", err) - } + return nil +} +func newDBConn() (*db.DB, error) { dbc, err := db.New(*confDBPath, db.DefaultOptions()) if err != nil { - log.Fatalf("error opening database: %v\n", err) + return nil, fmt.Errorf("error opening database: %w", err) } - defer dbc.Close() err = dbc.Migrate(db.MigrationContext{ Production: true, DBPath: *confDBPath, - OriginalMusicPath: confMusicPaths[0].path, + OriginalMusicPath: (*confMusicPaths)[0].path, PlaylistsPath: *confPlaylistsPath, PodcastsPath: *confPodcastPath, }) if err != nil { - log.Panicf("error migrating database: %v\n", err) + return nil, fmt.Errorf("error migrating database (PLEASE REPORT A BUG): %w", err) } + return dbc, nil +} + +func main() { + if err := loadConfig(); err != nil { + log.Fatal(err.Error()) + } + + cacheDirAudio := path.Join(*confCachePath, "audio") + cacheDirCovers := path.Join(*confCachePath, "covers") + if err := os.MkdirAll(cacheDirAudio, os.ModePerm); err != nil { + log.Fatalf("couldn't create audio cache path: %v\n", err) + } + if err := os.MkdirAll(cacheDirCovers, os.ModePerm); err != nil { + log.Fatalf("couldn't create covers cache path: %v\n", err) + } + + dbc, err := newDBConn() + if err != nil { + log.Fatal(err.Error()) + } + defer dbc.Close() + var musicPaths []ctrlsubsonic.MusicPath - for _, pa := range confMusicPaths { + for _, pa := range *confMusicPaths { musicPaths = append(musicPaths, ctrlsubsonic.MusicPath{Alias: pa.alias, Path: pa.path}) } @@ -161,7 +191,7 @@ func main() { *confProxyPrefix = proxyPrefixExpr.ReplaceAllString(*confProxyPrefix, `/$1`) if *deprecatedConfGenreSplit != "" && *deprecatedConfGenreSplit != "\n" { - confMultiValueGenre = multiValueSetting{Mode: scanner.Delim, Delim: *deprecatedConfGenreSplit} + *confMultiValueGenre = multiValueSetting{Mode: scanner.Delim, Delim: *deprecatedConfGenreSplit} *deprecatedConfGenreSplit = "" } if confMultiValueArtist.Mode == scanner.None && confMultiValueAlbumArtist.Mode > scanner.None { @@ -189,9 +219,9 @@ func main() { ctrlsubsonic.MusicPaths(musicPaths), dbc, map[scanner.Tag]scanner.MultiValueSetting{ - scanner.Genre: scanner.MultiValueSetting(confMultiValueGenre), - scanner.Artist: scanner.MultiValueSetting(confMultiValueArtist), - scanner.AlbumArtist: scanner.MultiValueSetting(confMultiValueAlbumArtist), + scanner.Genre: scanner.MultiValueSetting(*confMultiValueGenre), + scanner.Artist: scanner.MultiValueSetting(*confMultiValueArtist), + scanner.AlbumArtist: scanner.MultiValueSetting(*confMultiValueAlbumArtist), }, tagReader, *confExcludePattern, From 861f5293434b7bf199cef70ddb07766f001b45a0 Mon Sep 17 00:00:00 2001 From: ThinkChaos Date: Mon, 4 Mar 2024 20:11:10 -0500 Subject: [PATCH 2/5] fix(cmd): don't print `-version` flag value on startup --- cmd/gonic/gonic.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cmd/gonic/gonic.go b/cmd/gonic/gonic.go index a48ce6853..cbea54768 100644 --- a/cmd/gonic/gonic.go +++ b/cmd/gonic/gonic.go @@ -205,6 +205,11 @@ func main() { log.Printf("starting gonic v%s\n", gonic.Version) log.Printf("provided config\n") flag.VisitAll(func(f *flag.Flag) { + switch f.Name { + case "version": // always "false" + return + } + value := strings.ReplaceAll(f.Value.String(), "\n", "") log.Printf(" %-25s %s\n", f.Name, value) }) From 724ba37f15684c54f548ed7e17592926ace0f07a Mon Sep 17 00:00:00 2001 From: ThinkChaos Date: Mon, 4 Mar 2024 20:11:30 -0500 Subject: [PATCH 3/5] fix(cmd): clarify path configuration errors --- cmd/gonic/gonic.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/gonic/gonic.go b/cmd/gonic/gonic.go index cbea54768..23631faa9 100644 --- a/cmd/gonic/gonic.go +++ b/cmd/gonic/gonic.go @@ -524,10 +524,10 @@ func (pa *pathAliases) Set(value string) error { func validatePath(p string) (string, error) { if p == "" { - return "", errors.New("path can't be empty") + return "", errors.New("path configuration can't be empty") } if _, err := os.Stat(p); os.IsNotExist(err) { - return "", errors.New("path does not exist, please provide one") + return "", errors.New("path does not exist, please create it") } p, err := filepath.Abs(p) if err != nil { From b533ab75daba974985ece749e436a833fbfdfcd7 Mon Sep 17 00:00:00 2001 From: ThinkChaos Date: Mon, 4 Mar 2024 20:39:25 -0500 Subject: [PATCH 4/5] style: fix minor lint errors overridden with `nolint` --- cmd/gonic/gonic.go | 42 ++++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/cmd/gonic/gonic.go b/cmd/gonic/gonic.go index 23631faa9..410c16656 100644 --- a/cmd/gonic/gonic.go +++ b/cmd/gonic/gonic.go @@ -1,4 +1,3 @@ -//nolint:lll,gocyclo,forbidigo,nilerr,errcheck package main import ( @@ -49,6 +48,8 @@ import ( "go.senan.xyz/gonic/transcode" ) +var errNotConfigured = errors.New("not configured") + //nolint:gochecknoglobals var ( confListenAddr = flag.String("listen-addr", "0.0.0.0:4747", "listen address (optional)") @@ -106,11 +107,19 @@ func flagVar[T any, TPtr flagValue[T]](name, usage string) *T { func loadConfig() error { flag.Parse() - flagconf.ParseEnv() - flagconf.ParseConfig(*confConfigPath) + + err := flagconf.ParseEnv() + if err != nil { + return err + } + + err = flagconf.ParseConfig(*confConfigPath) + if err != nil { + return err + } if *confShowVersion { - fmt.Printf("v%s\n", gonic.Version) + fmt.Printf("v%s\n", gonic.Version) //nolint:forbidigo os.Exit(0) } @@ -122,7 +131,6 @@ func loadConfig() error { return errors.New("please provide a music directory") } - var err error for i, confMusicPath := range *confMusicPaths { if (*confMusicPaths)[i].path, err = validatePath(confMusicPath.path); err != nil { return fmt.Errorf("checking music dir %q: %w", confMusicPath.path, err) @@ -162,6 +170,7 @@ func newDBConn() (*db.DB, error) { return dbc, nil } +//nolint:gocyclo func main() { if err := loadConfig(); err != nil { log.Fatal(err.Error()) @@ -238,11 +247,20 @@ func main() { ) lastfmClientKeySecretFunc := func() (string, string, error) { - apiKey, _ := dbc.GetSetting(db.LastFMAPIKey) - secret, _ := dbc.GetSetting(db.LastFMSecret) + apiKey, err := dbc.GetSetting(db.LastFMAPIKey) + if err != nil { + return "", "", err + } + + secret, err := dbc.GetSetting(db.LastFMSecret) + if err != nil { + return "", "", err + } + if apiKey == "" || secret == "" { - return "", "", fmt.Errorf("not configured") + return "", "", errNotConfigured } + return apiKey, secret, nil } @@ -448,7 +466,11 @@ func main() { errgrp.Go(func() error { if _, _, err := lastfmClientKeySecretFunc(); err != nil { - return nil + if errors.Is(err, errNotConfigured) { + return nil //nolint:nilerr + } + + return err } defer logJob("refresh artist info")() @@ -491,7 +513,7 @@ func main() { log.Panic(err) } - fmt.Println("shutdown complete") + log.Println("shutdown complete") } const pathAliasSep = "->" From 1a287ce01f36c3b7185672b4572abd38ec06487d Mon Sep 17 00:00:00 2001 From: ThinkChaos Date: Mon, 4 Mar 2024 20:40:13 -0500 Subject: [PATCH 5/5] style: remove unused `nolint` and make some file level ones specific --- db/migrations.go | 1 - infocache/albuminfocache/albuminfocache.go | 1 - infocache/artistinfocache/artistinfocache.go | 1 - scanner/scanner.go | 3 +-- server/ctrladmin/handlers.go | 1 - transcode/transcode.go | 4 +++- version.go | 11 +++++++---- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/db/migrations.go b/db/migrations.go index 80304cf56..2c885872a 100644 --- a/db/migrations.go +++ b/db/migrations.go @@ -1,4 +1,3 @@ -//nolint:goerr113 package db import ( diff --git a/infocache/albuminfocache/albuminfocache.go b/infocache/albuminfocache/albuminfocache.go index f9c96774a..923280d1e 100644 --- a/infocache/albuminfocache/albuminfocache.go +++ b/infocache/albuminfocache/albuminfocache.go @@ -1,4 +1,3 @@ -//nolint:revive package albuminfocache import ( diff --git a/infocache/artistinfocache/artistinfocache.go b/infocache/artistinfocache/artistinfocache.go index f30c1cba5..6dcb0cde4 100644 --- a/infocache/artistinfocache/artistinfocache.go +++ b/infocache/artistinfocache/artistinfocache.go @@ -1,4 +1,3 @@ -//nolint:revive package artistinfocache import ( diff --git a/scanner/scanner.go b/scanner/scanner.go index fcadecaa6..d884a924b 100644 --- a/scanner/scanner.go +++ b/scanner/scanner.go @@ -1,4 +1,3 @@ -//nolint:nestif package scanner import ( @@ -328,7 +327,7 @@ func (s *Scanner) populateTrackAndArtists(tx *db.DB, st *State, i int, album *db } // metadata for the album table comes only from the first track's tags - if i == 0 { + if i == 0 { //nolint:nestif if err := tx.Where("album_id=?", album.ID).Delete(db.ArtistAppearances{}).Error; err != nil { return fmt.Errorf("delete artist appearances: %w", err) } diff --git a/server/ctrladmin/handlers.go b/server/ctrladmin/handlers.go index cb0819836..63e5c7450 100644 --- a/server/ctrladmin/handlers.go +++ b/server/ctrladmin/handlers.go @@ -1,4 +1,3 @@ -//nolint:goerr113 package ctrladmin import ( diff --git a/transcode/transcode.go b/transcode/transcode.go index d5b9fe1e0..1ecdc4340 100644 --- a/transcode/transcode.go +++ b/transcode/transcode.go @@ -1,7 +1,6 @@ // author: spijet (https://github.com/spijet/) // author: sentriz (https://github.com/sentriz/) -//nolint:gochecknoglobals package transcode import ( @@ -18,6 +17,7 @@ type Transcoder interface { Transcode(ctx context.Context, profile Profile, in string, out io.Writer) error } +//nolint:gochecknoglobals var UserProfiles = map[string]Profile{ "mp3": MP3, "mp3_320": MP3320, @@ -32,6 +32,8 @@ var UserProfiles = map[string]Profile{ } // Store as simple strings, since we may let the user provide their own profiles soon +// +//nolint:gochecknoglobals var ( MP3 = NewProfile("audio/mpeg", "mp3", 128, `ffmpeg -v 0 -i -ss -map 0:a:0 -vn -b:a -c:a libmp3lame -f mp3 -`) MP3320 = NewProfile("audio/mpeg", "mp3", 320, `ffmpeg -v 0 -i -ss -map 0:a:0 -vn -b:a -c:a libmp3lame -f mp3 -`) diff --git a/version.go b/version.go index 06c99af11..c896cac5c 100644 --- a/version.go +++ b/version.go @@ -1,4 +1,3 @@ -//nolint:gochecknoglobals,golint,stylecheck package gonic import ( @@ -6,9 +5,13 @@ import ( "strings" ) -//go:embed version.txt -var version string -var Version = strings.TrimSpace(version) +//nolint:gochecknoglobals +var ( + //go:embed version.txt + version string + + Version = strings.TrimSpace(version) +) const ( Name = "gonic"