-
Notifications
You must be signed in to change notification settings - Fork 157
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
chore(test): run go interop tests in the same rust process #4636
Conversation
972ca4a
to
cf560ae
Compare
…test-in-same-process
6b9598c
to
920d40e
Compare
920d40e
to
f3c1ca9
Compare
} | ||
|
||
#[derive(rust2go::R2G, Clone, Default)] | ||
pub struct EmptyReq {} |
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 required due to a bug ihciah/rust2go#10
…test-in-same-process
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.
The end binary doesn't require go
, right? That is to say, if we ship the forest
binary, there are no extra requirements (except the ones that are already listed in the Dockerfile)?
package main | ||
|
||
/* | ||
// Generated by rust2go. Please DO NOT edit this C part manually. |
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.
Should the generated file be included in the version control?
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'm neutral about it. It's not required but makes code readable for users who solely browse the repo. What's your recommendation?
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.
Pros:
- any change to it must be reviewed,
- somehow searchable.
Cons:
- will the changes to this file be actually reviewed or just blindly approved, especially when it'll be some obscure Go with C code?
- it introduces some noise, e.g., simple
Cargo.toml
or lockfiles are easily approved, - while it allows some searching, it will also make general grepping more difficult - in my workflow, I'd need to disable looking in
.go
files to avoid noise.
All in all, if it's trivial to generate, I'd generate it on the fly and not add it to the version control.
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.
Removed
@LesnyRumcajs Yes, no extra dependencies(unless go-f3 dynamically links to other libs). BTW, this change happens to a |
Co-authored-by: Hubert <[email protected]>
428d3f1
to
8303f88
Compare
Hi! I'm the author of rust2go. Thank you for choosing rustgo! In out production environment we have validated its relibility. One thing to note is that, remember to add For testing, it doesn't matter whether there is this environment variable or not. But if you use it to serve requests, remember to add it. |
@ihciah Thank you for the hints, this has been addressed in #4682 |
Summary of changes
This PR uses
rust2go
to invoke thego-kad
test app in the rust process, to demonstrate how FFI for the f3 sidecar works.Note that
go1.21.x
is required to runforest-interop-test
Changes introduced in this pull request:
go-kad
compact test toforest-interop-test
and invoke withrust2go
go-bitswap
compact test toforest-interop-test
and invoke withrust2go
Test result on CI: https://github.com/ChainSafe/forest/actions/runs/10355850383/job/28664406918?pr=4636#step:6:2191
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist