-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
add first version of rgr sandwich #3306
Conversation
@arlyon is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
You must have Developer access to commit code to Vercel on Vercel. If you contact an administrator and receive Developer access, commit again to see your changes. Learn more: https://vercel.com/docs/concepts/teams/roles-and-permissions#enterprise-team-account-roles |
e4e69b9
to
1e354f2
Compare
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Only a nit about some commented out code.
02f0623
to
e0ac68e
Compare
f52c854
to
39fc655
Compare
ef8cf24
to
39ddc71
Compare
This API is going to be used later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This landed while I was mid-review.
message GlobReq { | ||
string base_path = 1; | ||
repeated string include_patterns = 2; | ||
repeated string exclude_patterns = 3; | ||
bool files_only = 4; // note that the default for a bool is false | ||
} | ||
|
||
message GlobResp { | ||
oneof response { | ||
GlobRespList files = 1; | ||
string error = 2; | ||
} | ||
} | ||
|
||
message GlobRespList { | ||
repeated string files = 1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that these are unused right now? I believe we should drop them from this PR.
GO_FILES = $(shell find . -name "*.go") | ||
SRC_FILES = $(shell find . -name "*.go" | grep -v "_test.go") | ||
GENERATED_FILES = internal/turbodprotocol/turbod.pb.go internal/turbodprotocol/turbod_grpc.pb.go | ||
|
||
turbo: go-turbo$(EXT) | ||
cargo build --manifest-path ../crates/turborepo/Cargo.toml | ||
|
||
go-turbo$(EXT): $(GENERATED_FILES) $(SRC_FILES) go.mod | ||
CGO_ENABLED=1 go build $(GO_FLAGS) -o go-turbo$(EXT) ./cmd/turbo | ||
go-turbo$(EXT): $(GENERATED_FILES) $(SRC_FILES) go.mod turborepo-ffi-install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a transitive dependency, but Go doesn't know how to build it.
I don't have a strong opinion here.
|
||
.PHONY: turborepo-ffi-proto | ||
turborepo-ffi-proto: | ||
cd ../crates/turborepo-ffi && protoc --go_out=. messages.proto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mild preference for this, which is untested but I think it works:
cd ../crates/turborepo-ffi && protoc --go_out=. messages.proto | |
cd ../crates/turborepo-ffi && protoc --go_out=$(CLI_DIR)/ffi/proto messages.proto |
return nil | ||
} | ||
|
||
// Marshal consumes a proto.Message and returns a bufferfire |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a bufferfire similar to a dumpster fire?
) | ||
|
||
// Unmarshal consumes a buffer and parses it into a proto.Message | ||
func Unmarshal[M proto.Message](b C.Buffer, c M) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is our first generic into the Go codebase. I don't think it matters anymore, but I want to make sure we didn't just break linting.
/cc @gsoltis
) | ||
|
||
// Unmarshal consumes a buffer and parses it into a proto.Message | ||
func Unmarshal[M proto.Message](b C.Buffer, c M) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename these variables? We're not at a limit on characters and b C.Buffer, c M
is unnecessarily cryptic.
// rather than use C.GoBytes, we use this function to avoid copying the bytes, | ||
// since it is going to be immediately Unmarshalled into a proto.Message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is clever. I still kinda prefer GoBytes
in spite of the bonus copy.
Hi!
This is a proof-of-concept for the rust-go-rust sandwich. It successfully ports the
GetTurboDataDir
function onto the rust side, and exposes it via a single statically-linked libraryturborepo_ffi
along with bindings for it. It is expected that anything we need via FFI and the accompanying glue code can live here, so we can put the needed functionality into the right places in rust-land and just re-expose a simple API here.There are still a number of things that we need to do to finish this work:
swig
to generate the go bindings which needs to be re-run when changing any function signatures, so we'll have to decide if we want that to be a required part of the toolchain or optional. things that need to be ignored: libturborepo_ffi.a, bindings.h (?), messages.pb.go (?), swig_wrap.cxx (?), turborepo_ffi.go (?)update the build.rs file for turborepo to statically build and move(handled in the makefile)libturborepo_ffi
and associated artifacts into the go module-tag
(perhaps enable it by default for canary builds...?)That said, it's working on both ends using a nice generic interface.
turbo/crates/turborepo-ffi/src/lib.rs
Lines 5 to 32 in 66f3d52
turbo/cli/internal/ffi/util.go
Lines 10 to 26 in 66f3d52
Usage looks something like the following:
turbo/crates/turborepo-ffi/src/lib.rs
Lines 41 to 50 in 66f3d52
turbo/cli/internal/fs/get_turbo_data_dir_rust.go
Lines 11 to 18 in 66f3d52
Implementation Details
GO_TAG=rust
How to try it out:
The rust implementation of GetTurboDataDir will print out the path whenever it is called, to verifiy it is being used.