Refactor Pebble internals into an exportable package

[anpep]

This PR guides Pebble towards an internal structure that enables us to create Pebble-based applications that implement custom commands and add functionality that is not relevant to the upstream Pebble repository. However, we don't want to create a full-blown fork of Pebble and have the maintenance burden of having to port changes from upstream to every fork; for this reason, this PR does two things:

• Rename the internal package to internals so that it can be imported by other Pebble-based applications.
• Refactor all CLI functionality from the main package to the internals/cli package, so that other Pebble-based applications can import this package without having to reimplement this functionality.

This PR does not intend to be exhaustive and instead aims to serve as the groundwork for achieving the described goal. Thanks for your reviews!

[flotter]

Note to other reviewers: This file is a verbatim copy from everything that did not stay in cmd/pebble/main.go above. The main difference is the deferred recovery() hook is now moved one level up into Run().

[niemeyer]

Thanks for the helpful observation.

[flotter]

LGTM. Two minor corrections I think.

[benhoyt]

Can you show how you would actually create a new binary that adds (or removes) commands? Is the intention to rewrite cli.Run? But even then, how would you add your new commands? addCommand isn't exported, so it would seem to me at the least we'd need to export that function.

It also seems to me different binaries might want to have different subsets of commands, that is, to remove existing Pebble commands. And currently all the addCommand calls are done inside init() functions. That seems non-ideal, as they're already all added before the package importer gets a chance to adjust them.

What about having an explicit list in main(), something like? (I'm not recommending this exact API.)

func main() {
    cli.AddCommand(cli.Add)
    cli.AddCommand(cli.Autostart)
    cli.AddCommand(cli.Changes)
    /* ... add other commands ... */
    cli.AddCommand(cli.CmdInfo{  // add our custom command defined here
		Name:      "custom",
		ShortHelp: "short help",
		LongHelp:  "long help",
		Builder:   func() flags.Commander { return &cmdCustom{} },
		OptDescs:  customDescs,
	}
    })
    cli.Run() // error handling omitted
}

We'd also need to rework how to generate the help categories a bit, to make that adjustable.

[benhoyt]

A couple of other things:

1. Do we need to export all of the internal packages like you're doing, or can we just export the cli package? Do we need to access internal details of the other packages?
2. I wonder if the name internals is a bit confusing, as it's too close to internal, which has special meaning (but of course internals doesn't). Perhaps "private"? (Or maybe naming has been discussed already.)

[anpep]

Can you show how you would actually create a new binary that adds (or removes) commands? Is the intention to rewrite cli.Run? But even then, how would you add your new commands? addCommand isn't exported, so it would seem to me at the least we'd need to export that function.

I have a working implementation of this which basically exports addCommand and ways to append a help category to the manual (as Gustavo suggested), however, I'm yet to make a good proposal since I wanted to limit the scope of this PR to just exporting the internal package.

It also seems to me different binaries might want to have different subsets of commands, that is, to remove existing Pebble commands. And currently all the addCommand calls are done inside init() functions. That seems non-ideal, as they're already all added before the package importer gets a chance to adjust them.

I think you have a valid point here; however, I'm not sure removing "core" functionality from Pebble is a desired feature, actually, the aim is quite the opposite: to move functionality that's not common to all Pebble stakeholders out of the upstream Pebble repository and extend it in other applications (currently with Termus specifically in mind).

What about having an explicit list in main(), something like? (I'm not recommending this exact API.)

I will try to make a proposal tomorrow and gather feedback from Gustavo before going down this rabbit hole (: But this seems like a sensible option we can discuss and evolve over time.

A couple of other things:

1. Do we need to export all of the internal packages like you're doing, or can we just export the cli package? Do we need to access internal details of the other packages?

I think this is the way to go, since we might need access to e.g. osutil from within commands that have very specific functionality. Or, thinking about Termus, we might want to have direct access to the overlord package so that we can instantiate an overlord, start the daemon, etc.

With this in mind, it makes sense for the cli package to be inside internals and not cmd/pebble, for instance.

2. I wonder if the name internals is a bit confusing, as it's too close to internal, which has special meaning (but of course internals doesn't). Perhaps "private"? (Or maybe naming has been discussed already.)

Naming this package has proven to be very tricky, but after some discussion, Gustavo suggested internals which seems to work for us because: (1) makes it obvious that this package exposes a private API and thus prone to change, and (2) it's close enough to the original word internal that doesn't trick people into thinking this package has a different scope than the original.

As always, really appreciate your feedback!

[benhoyt]

@anpep All fair enough, thanks! Naming is hard. :-)

[niemeyer]

Thanks for the nice review, @benhoyt. As Ángel points out, the close proximity with "internal" is intentional here. Idelly we want people to feel exactly the same way about both, as for now there's a single exception that we'll be respecting.

[niemeyer]

That looks good, Ángel, thank you. Just one trivial comment for consideration in the future.

This merge will likely break most open PRs, unfortunately, but there's no way around it.

[niemeyer]

Do we still need this?