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

[pyth_oracle] Improve build.rs robustness and expose types. #343

Merged
merged 3 commits into from
Apr 10, 2023

Conversation

Reisen
Copy link
Contributor

@Reisen Reisen commented Apr 6, 2023

This PR adds a feature flag, library to the pyth-oracle program. When toggled on, the library exposes its account types for use as a library instead of a standalone program. When the flag is enabled, the build.rs script will also switch to execute the C compilation as well. The artefacts are output within the Rust target sandbox rather than into the relative source directory (./program/c/target).

The benefit of this:

  • The build is now properly sandboxed within the Rust build directory.
  • The C is built automatically with Rust.
  • The library can now be used by other programs, and the C builds successfully.

Rather than fully switch to this, the feature flag is added so any existing reliance on the current build method continues to work. The primary reason I wanted this is because I am relying on pyth-sdk-rs and I found it very hard to tell exactly what is different there versus the accounts defined within the contract. With this feature flag it is possible for me to enforce extra constraints in pyth-sdk-rs that prevent the libraries going out of sync and highlights the differences in a testable way.

See the associated PR for context.

jayantk
jayantk previously approved these changes Apr 6, 2023
Copy link
Contributor

@jayantk jayantk left a comment

Choose a reason for hiding this comment

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

very cool.

println!("cargo:rustc-link-search=./program/c/target");
// Cargo exposes ENV variables for feature flags that can be used to determine whether to do a
// self-contained build that compiles the C components automatically.
if std::env::var("CARGO_FEATURE_LIBRARY").is_ok() {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any reason not to do this for both build paths? this seems like a great simplification of the current build 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.

No reason, this is the correct way to do it. But I wasn't sure how many things would break and this was a safe small PR to get a review on. I can make it the default and check anywhere that might break / has been relying on relative paths. I just didn't plan to do it in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's get this merged and I can make it the default

guibescos
guibescos previously approved these changes Apr 6, 2023
Copy link
Contributor

@guibescos guibescos left a comment

Choose a reason for hiding this comment

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

LGTM, please fix ci

@jayantk jayantk dismissed stale reviews from guibescos and themself via 73073f9 April 10, 2023 17:38
@guibescos guibescos merged commit 659524d into main Apr 10, 2023
@guibescos guibescos deleted the reisen/pyth-oracle-build branch April 10, 2023 18:32
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.

3 participants