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

feat(abigen): emit cargo build script directives #2449

Merged
merged 1 commit into from Jun 1, 2023

Conversation

DaniPopes
Copy link
Collaborator

Motivation

... and an include_bytes! when not in build scripts. Signals Cargo to rebuild if the source files change.

Solution

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

nice, smol nit re error message otherwise lgtm

let contents = fs::read_to_string(&path).wrap_err("Could not read file")?;

Self::new(name, contents)
let path = path.as_ref().to_str().ok_or_else(|| eyre::eyre!("invalid path"))?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: error should include the path

/// Specify whether to print [cargo build script directives][ref] if the source is a path. By
/// default, this is true only when executing inside of a build script.
///
/// [ref]: https://doc.rust-lang.org/cargo/reference/build-scripts.html#outputs-of-the-build-script
Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to adapt this href style

@@ -368,6 +396,11 @@ impl ContractBindings {
}
}

/// Returns whether the current executable is a cargo build script.
fn in_build_script() -> bool {
std::env::var("CARGO_MAKEFLAGS").is_ok()
Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

honestly i just went over all of the env vars, and this one was the only one that is set only on build scripts, per docs. Not 100% sure

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TARGET should be fine as well.

@DaniPopes DaniPopes force-pushed the feat/incremental-abigen branch 2 times, most recently from 89d392b to f6ce0d8 Compare May 31, 2023 22:10
@DaniPopes DaniPopes merged commit 401a47d into gakonst:master Jun 1, 2023
19 checks passed
@DaniPopes DaniPopes deleted the feat/incremental-abigen branch June 1, 2023 06:50
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.

None yet

2 participants