Skip to content

Conversation

@ihciah
Copy link
Owner

@ihciah ihciah commented Jun 9, 2025

Fixes #83

@ihciah ihciah requested a review from Copilot June 9, 2025 00:26
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds support for resetting the generated Go header’s access and modification times to speed up rebuilds, exposes a new multi_param_test binding on the Go side, bumps the crate version, and renames example binaries for clarity.

  • Introduce multi_param_test on the Go interface and its CTestCall export
  • Update build.rs to read and restore header file timestamps around the Go build step
  • Bump rust2go to v0.4.2, add fs-set-times dependency, and rename example binary targets

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

File Description
test/go/gen.go Add multi_param_test to TestCall and implement its CTestCall export function
rust2go/src/build.rs Rename filenamelib_filename, add header_filename and read_header_file, and restore .h atime/mtime
rust2go/Cargo.toml Bump version to 0.4.2, add optional fs-set-times dependency, include it in build feature
examples/*/Cargo.toml Rename example binaries (e.g., exampleexample-tokio-cgo, etc.)
Comments suppressed due to low confidence (4)

test/go/gen.go:153

  • [nitpick] Variable name _new_user is unconventional in Go—consider renaming to userVal or userArg to follow Go naming conventions and improve readability.
_new_user := newUser(user)

rust2go/src/build.rs:357

  • LIB_EXT is not defined or imported here, leading to a compilation error. Define or import the correct constant (e.g., ".a") or replace with the appropriate static-library suffix.
LinkType::Static => format!("{DLL_PREFIX}go{LIB_EXT}"),

test/go/gen.go:151

  • No tests are added to cover the new multi_param_test export path—consider adding a Go-side or integration test to ensure this path is exercised and prevent regressions.
//export CTestCall_multi_param_test

rust2go/src/build.rs:370

  • [nitpick] Helper read_header_file would benefit from a doc comment explaining its return tuple (header bytes and optional access/modify times) to improve maintainability.
fn read_header_file(

@ihciah ihciah requested a review from lirenjie95 June 9, 2025 00:29
@ihciah
Copy link
Owner Author

ihciah commented Jun 9, 2025

@zhongxinghong Could you have a look at if this branch work? Thank you

Copy link
Collaborator

@lirenjie95 lirenjie95 left a comment

Choose a reason for hiding this comment

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

LGTM

@zhongxinghong
Copy link
Contributor

Hi~ I've tested the compile-cache-improve branch in our project. The times of libgo.h is correct now. But there still have another file that should be considered.

       Dirty r2g v0.1.0 (/a/b/c/ffi_wrapper/r2g): the file `ffi_wrapper/r2g/../wrapper` has changed (1749714290.758855671s, 32000095ns after last build at 1749714290.726855576s)
   Compiling r2g v0.1.0 (/a/b/c/ffi_wrapper/r2g)

rust2go regen the gen.go in every build and it also watches the go_src folder. Writing the gen.go to the go_src folder will changes the times of go_src folder, which causes things to be rebuild next time.

fn main() {
    rust2go::Builder::new()
        .with_go_src("../wrapper")
        .with_regen_arg(RegenArgs {
            src: "./src/idl.rs".into(),
            dst: "../wrapper/gen.go".into(),
            go118: true,
            ..Default::default()
        })
        .build();
}

My solution on the issue is to reset the times of wrapper/ and wrapper/gen.go nodes too, but I think there should have some better solutions. In our project, we can't control the re-generation of gen.go file, but in rust2go I think it can compare the file content before writing the gen.go to disk. I've checked the source code, I think that rust2go can generate gen.go to a tmpfile first and run go fmt on it. After that, compare the tmpfile content with the original gen.go (if it's existed). This should prevent the unnecessary changes of times of gen.go and its folder if its file content doesn't change in that build.

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.

cgo changes the time of libgo.h even if its content doesn't changed, causes build.rs rerun in every cargo check

4 participants