Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experimental compiler refactor #133

Merged
merged 29 commits into from
Jun 25, 2022
Merged

Experimental compiler refactor #133

merged 29 commits into from
Jun 25, 2022

Conversation

matthewmueller
Copy link
Contributor

@matthewmueller matthewmueller commented Jun 6, 2022

I'd like to take one more crack at the compiler refactor. It's important to get this process right because all the generators will fit within this overall process. Writing generators is the easy part.

The goal is to:

  • Transition from bud starting a long-running generated cli process that starts another long-running app process to bud optionally running a short-lived generated cli process, then running a long-running app process.

Additionally:

  • Gather all the generators and runtimes into a simple framework/ directory

This will hopefully make the code base much easier to follow, without compromising on flexibility.

@matthewmueller matthewmueller marked this pull request as draft June 6, 2022 06:27
@matthewmueller matthewmueller changed the title Experimental refactor Experimental compiler refactor Jun 6, 2022
@CLAassistant
Copy link

CLAassistant commented Jun 23, 2022

CLA assistant check
All committers have signed the CLA.

@matthewmueller matthewmueller marked this pull request as ready for review June 23, 2022 04:03
@matthewmueller
Copy link
Contributor Author

Woohoo, it's aliveeeeee. Just need to do a bit more cleanup and it's good to go!

# main branch. See: https://github.com/golang/go/issues/53226
- name: Install bud binary into $PATH
run: GOPRIVATE=* go install github.com/livebud/bud@main

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer necessary! There's no more spawning of a v8server when using bud run. Instead we've combined this with an internal local "bud server", that handles server-side rendering, hot reloads, and bundling client-side files.

This bud server is only run with bud run, when you bud build v8 is bundled, there's no hot reloads and all client-side files are pre-build and embedded.

@@ -50,7 +50,7 @@ example.hn.watch:
# Go
##

GO_SOURCE := ./internal/... ./package/... ./runtime/...
GO_SOURCE := ./internal/... ./package/... ./framework/...
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to combine internal/generator and runtime/generator into one package so that would put all the generators in one place. Framework felt like a better word for this because not all the packages in framework/ used at runtime, though I'm still tempted to change it to generator/

@@ -0,0 +1,45 @@
package app

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

app is the new entrypoint into the program. In the previous bud, there was a generated main and program. This was over-engineering at this point in bud's journey, so I opted to simplify things for the time being.

return err
}
// Inform bud that we're ready
budClient.Publish("app:ready", nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bud client talks to the server during bud run and informs the server that the app is ready. This allows tests to be less flaky and fail faster.

@@ -5,8 +5,8 @@ import (
"net/http/httptest"
"testing"

. "github.com/livebud/bud/framework/controller/controllerrt/request"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not totally happy with controllerrt, but it's short for "controller runtime"

}

// Module finds the go.mod file for the application
func Module(dir string) (*gomod.Module, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

internal/cli/bud contains some shared configuration, the root command and some helper functions that are used by the rest of the CLI

"github.com/livebud/bud/internal/cli/bud"
)

func New(bud *bud.Command, in *bud.Input) *Command {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some tools to inspect the generated files

}
// Print the archive to stdout
fmt.Fprintln(os.Stdout, string(txtar.Format(ar)))
return nil
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is going to be handy for reproing issues

"os/exec"

"github.com/livebud/bud/internal/once"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This package simplifies the previous exe package to consolidate how subprocesses are managed in bud. Instead of wrapping exec.Command, it can turn an exec.Cmd into a Process.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, it can allow you to configure commands earlier with shared env / stdio / files etc. that can be spawned at a later time.

return fmt.Sprintf(format(message), args...)
}

func Errorf(message string, args ...interface{}) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a way to add multi-line errors more easily.

@matthewmueller
Copy link
Contributor Author

matthewmueller commented Jun 25, 2022

This PR concludes a large internal refactor I've been working on since launch. The goals of this PR:

  • Make it easier to understand. The double generated binary building was something that often confused me.
  • Make it easier to contribute. I'm so impressed with the contributions so far, with this refactor it should be even easier.
  • Make it faster during development. By far the slowest step in the build process is running go build. If we didn't need to do the double-compile, it should be 2x faster.

In general, now that I'm starting to get a better understanding of what people are expecting from the framework, I can start moving the codebase more towards that.

The results of this PR:

  • There's no more generated cli, we're just generating an app! We will find solutions when the time comes for all the planned features that led to the generated cli design.
  • All generators and helper runtimes live in framework/, supporting packages that are needed by the generated code live in packages/, everything else goes in internal/.
  • Tests are faster and less flaky because no more generated CLI and a pubsub system that will tell the test when the app is ready
  • Tests are more end-to-end because they only support two commands: cli.Run and cli.Start.
  • No more v8client/v8server, everything has been consolidated into the bud client/server that's run during bud run.
  • Debug logging! Added --log=debug to bud and the test suite. Slowly passing the logger through the subsystems.

Since so much of the codebase has changed, I'm pretty sure there's going to be a few minor regressions, but we can address them as they pop up. The core workflows have remained unchanged.

This PR pretty much wraps up v0.2. Have a look at the discussion to learn more about next steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants