Switch to standard lib log.Logger & support dfile

- --debug-file flag can now be used in conjuction with --debug to
  specify the debug file path
- Push out logger concerns to callers of liveshare

internal/codespaces/codespaces.go

@@ -12,10 +12,16 @@ import (
 
 type logger interface {
 	Print(v ...interface{}) (int, error)
-	Printf(f string, v ...interface{}) (int, error)
 	Println(v ...interface{}) (int, error)
 }
 
+// TODO(josebalius): clean this up once we standardrize
+// logging for codespaces
+type liveshareLogger interface {
+	Println(v ...interface{})
+	Printf(f string, v ...interface{})
+}
+
 func connectionReady(codespace *api.Codespace) bool {
 	return codespace.Environment.Connection.SessionID != "" &&
 		codespace.Environment.Connection.SessionToken != "" &&
@@ -31,7 +37,7 @@ type apiClient interface {
 
 // ConnectToLiveshare waits for a Codespace to become running,
 // and connects to it using a Live Share session.
-func ConnectToLiveshare(ctx context.Context, log, sessionLogger logger, apiClient apiClient, codespace *api.Codespace) (*liveshare.Session, error) {
+func ConnectToLiveshare(ctx context.Context, log logger, sessionLogger liveshareLogger, apiClient apiClient, codespace *api.Codespace) (*liveshare.Session, error) {
 	var startedCodespace bool
 	if codespace.Environment.State != api.CodespaceEnvironmentStateAvailable {
 		startedCodespace = true

internal/codespaces/states.go

@@ -5,6 +5,8 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"io/ioutil"
+	"log"
 	"net"
 	"strings"
 	"time"
@@ -36,8 +38,10 @@ type PostCreateState struct {
 // PollPostCreateStates watches for state changes in a codespace,
 // and calls the supplied poller for each batch of state changes.
 // It runs until it encounters an error, including cancellation of the context.
-func PollPostCreateStates(ctx context.Context, log logger, apiClient apiClient, codespace *api.Codespace, poller func([]PostCreateState)) (err error) {
-	session, err := ConnectToLiveshare(ctx, log, nil, apiClient, codespace)
+func PollPostCreateStates(ctx context.Context, logger logger, apiClient apiClient, codespace *api.Codespace, poller func([]PostCreateState)) (err error) {
+	noopLogger := log.New(ioutil.Discard, "", 0)
+
+	session, err := ConnectToLiveshare(ctx, logger, noopLogger, apiClient, codespace)
 	if err != nil {
 		return fmt.Errorf("connect to Live Share: %w", err)
 	}
@@ -54,7 +58,7 @@ func PollPostCreateStates(ctx context.Context, log logger, apiClient apiClient,
 	}
 	localPort := listen.Addr().(*net.TCPAddr).Port
 
-	log.Println("Fetching SSH Details...")
+	logger.Println("Fetching SSH Details...")
 	remoteSSHServerPort, sshUser, err := session.StartSSHServer(ctx)
 	if err != nil {
 		return fmt.Errorf("error getting ssh server details: %w", err)

pkg/cmd/codespace/common.go

@@ -7,6 +7,8 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/ioutil"
+	"log"
 	"os"
 	"sort"
 	"strings"
@@ -211,6 +213,10 @@ func noArgsConstraint(cmd *cobra.Command, args []string) error {
 	return nil
 }
 
+func noopLogger() *log.Logger {
+	return log.New(ioutil.Discard, "", 0)
+}
+
 type codespace struct {
 	*api.Codespace
 }

pkg/cmd/codespace/logs.go

@@ -51,7 +51,7 @@ func (a *App) Logs(ctx context.Context, codespaceName string, follow bool) (err
 		return fmt.Errorf("get or choose codespace: %w", err)
 	}
 
-	session, err := codespaces.ConnectToLiveshare(ctx, a.logger, nil, a.apiClient, codespace)
+	session, err := codespaces.ConnectToLiveshare(ctx, a.logger, noopLogger(), a.apiClient, codespace)
 	if err != nil {
 		return fmt.Errorf("connecting to Live Share: %w", err)
 	}

pkg/cmd/codespace/ports.go

@@ -60,7 +60,7 @@ func (a *App) ListPorts(ctx context.Context, codespaceName string, asJSON bool)
 
 	devContainerCh := getDevContainer(ctx, a.apiClient, codespace)
 
-	session, err := codespaces.ConnectToLiveshare(ctx, a.logger, nil, a.apiClient, codespace)
+	session, err := codespaces.ConnectToLiveshare(ctx, a.logger, noopLogger(), a.apiClient, codespace)
 	if err != nil {
 		return fmt.Errorf("error connecting to Live Share: %w", err)
 	}
@@ -194,7 +194,7 @@ func (a *App) UpdatePortVisibility(ctx context.Context, codespaceName, sourcePor
 		return fmt.Errorf("error getting codespace: %w", err)
 	}
 
-	session, err := codespaces.ConnectToLiveshare(ctx, a.logger, nil, a.apiClient, codespace)
+	session, err := codespaces.ConnectToLiveshare(ctx, a.logger, noopLogger(), a.apiClient, codespace)
 	if err != nil {
 		return fmt.Errorf("error connecting to Live Share: %w", err)
 	}
@@ -253,7 +253,7 @@ func (a *App) ForwardPorts(ctx context.Context, codespaceName string, ports []st
 		return fmt.Errorf("error getting codespace: %w", err)
 	}
 
-	session, err := codespaces.ConnectToLiveshare(ctx, a.logger, nil, a.apiClient, codespace)
+	session, err := codespaces.ConnectToLiveshare(ctx, a.logger, noopLogger(), a.apiClient, codespace)
 	if err != nil {
 		return fmt.Errorf("error connecting to Live Share: %w", err)
 	}

pkg/cmd/codespace/ssh.go

@@ -4,11 +4,11 @@ import (
 	"context"
 	"fmt"
 	"io/ioutil"
+	"log"
 	"net"
 	"os"
 
 	"github.com/cli/cli/v2/internal/codespaces"
-	"github.com/cli/cli/v2/pkg/cmd/codespace/output"
 	"github.com/cli/cli/v2/pkg/liveshare"
 	"github.com/spf13/cobra"
 )
@@ -18,6 +18,7 @@ type sshOptions struct {
 	profile    string
 	serverPort int
 	debug      bool
+	debugFile  string
 }
 
 func newSSHCmd(app *App) *cobra.Command {
@@ -35,6 +36,7 @@ func newSSHCmd(app *App) *cobra.Command {
 	sshCmd.Flags().IntVarP(&opts.serverPort, "server-port", "", 0, "SSH server port number (0 => pick unused)")
 	sshCmd.Flags().StringVarP(&opts.codespace, "codespace", "c", "", "Name of the codespace")
 	sshCmd.Flags().BoolVarP(&opts.debug, "debug", "d", false, "Log debug data to a file")
+	sshCmd.Flags().StringVarP(&opts.debugFile, "debug-file", "", "", "Path of the file log to")
 
 	return sshCmd
 }
@@ -62,7 +64,7 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e
 
 	var debugLogger *fileLogger
 	if opts.debug {
-		debugLogger, err = newFileLogger("gh-cs-ssh")
+		debugLogger, err = newFileLogger(opts.debugFile)
 		if err != nil {
 			return fmt.Errorf("error creating debug logger: %w", err)
 		}
@@ -125,26 +127,34 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e
 	}
 }
 
-// fileLogger is a wrapper around an output.Logger configured to write
+// fileLogger is a wrapper around an log.Logger configured to write
 // to a file. It exports two additional methods to get the log file name
 // and close the file handle when the operation is finished.
 type fileLogger struct {
-	// TODO(josebalius): should we use https://pkg.go.dev/log#New instead?
-	*output.Logger
+	*log.Logger
 
 	f *os.File
 }
 
 // newFileLogger creates a new fileLogger. It returns an error if the file
-// cannot be created. The file is created in the operating system tmp directory
-// under the name parameter.
-func newFileLogger(name string) (*fileLogger, error) {
-	f, err := ioutil.TempFile("", name)
-	if err != nil {
-		return nil, err
+// cannot be created. The file is created on the specified path, if the path
+// is empty it is created in the temporary directory.
+func newFileLogger(file string) (fl *fileLogger, err error) {
+	var f *os.File
+	if file == "" {
+		f, err = ioutil.TempFile("", "")
+		if err != nil {
+			return nil, fmt.Errorf("failed to create tmp file: %w", err)
+		}
+	} else {
+		f, err = os.Create(file)
+		if err != nil {
+			return nil, err
+		}
 	}
+
 	return &fileLogger{
-		Logger: output.NewLogger(f, f, false),
+		Logger: log.New(f, "", log.LstdFlags),
 		f:      f,
 	}, nil
 }

pkg/liveshare/client.go

@@ -24,18 +24,8 @@ import (
 )
 
 type logger interface {
-	Println(v ...interface{}) (int, error)
-	Printf(f string, v ...interface{}) (int, error)
-}
-
-type noopLogger struct{}
-
-func (n noopLogger) Println(...interface{}) (int, error) {
-	return 0, nil
-}
-
-func (n noopLogger) Printf(string, ...interface{}) (int, error) {
-	return 0, nil
+	Println(v ...interface{})
+	Printf(f string, v ...interface{})
 }
 
 // An Options specifies Live Share connection parameters.
@@ -46,8 +36,8 @@ type Options struct {
 	RelaySAS       string
 	RelayEndpoint  string
 	HostPublicKeys []string
+	Logger         logger      // required
 	TLSConfig      *tls.Config // (optional)
-	Logger         logger      // (optional)
 }
 
 // uri returns a websocket URL for the specified options.
@@ -85,9 +75,8 @@ func Connect(ctx context.Context, opts Options) (*Session, error) {
 		return nil, err
 	}
 
-	var sessionLogger logger = noopLogger{}
-	if opts.Logger != nil {
-		sessionLogger = opts.Logger
+	if opts.Logger == nil {
+		return nil, errors.New("Logger is required")
 	}
 
 	sock := newSocket(uri, opts.TLSConfig)
@@ -124,7 +113,7 @@ func Connect(ctx context.Context, opts Options) (*Session, error) {
 		rpc:             rpc,
 		clientName:      opts.ClientName,
 		keepAliveReason: make(chan string, 1),
-		logger:          sessionLogger,
+		logger:          opts.Logger,
 	}
 	go s.heartbeat(ctx, 1*time.Minute)
 

pkg/liveshare/client_test.go

@@ -20,6 +20,7 @@ func TestConnect(t *testing.T) {
 		SessionToken:   "session-token",
 		RelaySAS:       "relay-sas",
 		HostPublicKeys: []string{livesharetest.SSHPublicKey},
+		Logger:         newMockLogger(),
 	}
 	joinWorkspace := func(req *jsonrpc2.Request) (interface{}, error) {
 		var joinWorkspaceReq joinWorkspaceArgs

pkg/liveshare/session_test.go

@@ -41,6 +41,7 @@ func makeMockSession(opts ...livesharetest.ServerOption) (*livesharetest.Server,
 		RelaySAS:       "relay-sas",
 		HostPublicKeys: []string{livesharetest.SSHPublicKey},
 		TLSConfig:      &tls.Config{InsecureSkipVerify: true},
+		Logger:         newMockLogger(),
 	})
 	if err != nil {
 		return nil, nil, fmt.Errorf("error connecting to Live Share: %w", err)
@@ -383,16 +384,16 @@ func newMockLogger() *mockLogger {
 	return &mockLogger{buf: new(bytes.Buffer)}
 }
 
-func (m *mockLogger) Printf(format string, v ...interface{}) (int, error) {
+func (m *mockLogger) Printf(format string, v ...interface{}) {
 	m.Lock()
 	defer m.Unlock()
-	return m.buf.WriteString(fmt.Sprintf(format, v...))
+	m.buf.WriteString(fmt.Sprintf(format, v...))
 }
 
-func (m *mockLogger) Println(v ...interface{}) (int, error) {
+func (m *mockLogger) Println(v ...interface{}) {
 	m.Lock()
 	defer m.Unlock()
-	return m.buf.WriteString(fmt.Sprintln(v...))
+	m.buf.WriteString(fmt.Sprintln(v...))
 }
 
 func (m *mockLogger) String() string {