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

Align C FFI with overview #922

Merged
merged 7 commits into from
Mar 8, 2025
Merged

Align C FFI with overview #922

merged 7 commits into from
Mar 8, 2025

Conversation

andrzejressel
Copy link
Owner

@andrzejressel andrzejressel commented Mar 7, 2025

Closes #896

Copy link

codecov bot commented Mar 7, 2025

Codecov Report

Attention: Patch coverage is 0% with 152 lines in your changes missing coverage. Please review.

Project coverage is 48.73%. Comparing base (16a9d39) to head (38924ab).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
crates/c_ffi/src/lib.rs 0.00% 152 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #922      +/-   ##
==========================================
- Coverage   49.17%   48.73%   -0.45%     
==========================================
  Files          63       63              
  Lines        8821     8901      +80     
==========================================
  Hits         4338     4338              
- Misses       4483     4563      +80     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrzejressel andrzejressel marked this pull request as draft March 8, 2025 11:59
@andrzejressel andrzejressel requested a review from Copilot March 8, 2025 17:19
Copy link

@Copilot 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.

PR Overview

This PR updates the Pulumi C FFI and example code to align with the new API overview. Key changes include:

  • Adding a new function, get_array_as_string, to the common library for stack exports.
  • Renaming and refactoring C FFI types and functions (e.g. replacing CustomRegisterOutputId with CustomCompositeOutputId and PulumiEngine with PulumiContext).
  • Updating test cases and configuration (in C++ and Docker examples, as well as cbindgen.toml) to reflect the new naming and API structure.

Reviewed Changes

File Description
examples/common/src/lib.rs Added a new function to serialize an array from the stack export.
crates/c_ffi/src/lib.rs Refactored types and functions to align with the new FFI design, including renaming and API changes.
crates/c_ffi/Cargo.toml Removed unused workspace dependencies to match the updated design.
examples/cpp/tests/test.rs Updated tests to use the new get_array_as_string and additional output assertions.
crates/c_ffi/cbindgen.toml Updated exported names to reflect the new type names.
examples/docker/tests/test.rs Modified tests to assert against repo_digest instead of remote_image_id.
examples/docker/src/lib.rs Renamed export key from remote_image_id to repo_digest.

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

crates/c_ffi/src/lib.rs:189

  • An unresolved FIXME remains in the invoke resource function, which may indicate incomplete output management. Consider addressing this issue to ensure proper handling.
    // inner_engine.outputs.push(raw); //FIXME

ctx: Rc::downgrade(inner),
};

// inner_engine.outputs.push(raw); //FIXME
Copy link
Preview

Copilot AI Mar 8, 2025

Choose a reason for hiding this comment

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

There is an unresolved FIXME comment regarding output registration. Please either implement the necessary logic or remove the comment if it's no longer applicable.

Suggested change
// inner_engine.outputs.push(raw); //FIXME
inner_engine.outputs.push(output.native.clone());

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@andrzejressel andrzejressel added the ready-to-merge Will be auto-merged by mergify label Mar 8, 2025
@andrzejressel andrzejressel marked this pull request as ready for review March 8, 2025 18:18
@mergify mergify bot merged commit c0e079d into main Mar 8, 2025
103 checks passed
@mergify mergify bot deleted the c_ffi_align branch March 8, 2025 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Will be auto-merged by mergify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align C FFI integration with overview
1 participant