-
Notifications
You must be signed in to change notification settings - Fork 253
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
fix: __abi-embed
compilation error
#971
Conversation
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.
Wait, so if we can just depend on features that are not declared in Cargo.toml
, couldn't we just remove __abi-embed
and __abi-generate
from Cargo.toml
?
$ cargo build --features near-sdk/__abi-generate
error: failed to select a version for `near-sdk`.
... required by package `adder v0.1.0 (/Users/miraclx/git/near/near-sdk-rs/examples/adder)`
versions that meet the requirements `*` (locked to 4.1.1) are: 4.1.1
the package `adder` depends on `near-sdk`, with features: `__abi-generate` but `near-sdk` does not have these features.
failed to select a version for `near-sdk` which could resolve this conflict |
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.
A little hacky, but looks reasonable to me! 👍
fn main() { | ||
if cfg!(feature = "__abi-embed") { | ||
if option_env!("CARGO_NEAR_ABI_PATH").is_some() { | ||
println!("cargo:rustc-cfg=feature=\"__abi-embed-checked\""); |
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.
curious -- I wonder if it is possible to manually pass the feature flag to the compiler from outside of the build script? This way we would be able to remove the pseudo-private flags altogether since the overhead doesn't matter for cargo-near?
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.
You mean through RUSTFLAGS
? That applies to all rustc
instances cargo spawns throughout its execution. Can't guarantee anything else (deps) maintains its behavior with that flag activated. But through cargo
, we constrain the flag activation to the sdk crate in particular. And then only conditionally activate __abi-embed-checked
, also specifically for the crate.
An alternative I experimented with early on, was to use cargo-near
as RUSTC_WRAPPER
, so when generating ABI, it embeds a unique identifier for the sdk in it's chunks. And when embedding the ABI, it selectively injects the embed flag specifically for the SDK.
But that was cumbersome, so I stripped it.
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.
Can't guarantee anything else (deps) maintains its behavior with that flag activated
What would be the concern there, that there would be a dependency used in a contract being built that for some reason uses a __abi-embed
feature flag?
I think this solution is good enough for now, but maybe we could have at least a tracking issue to see if we can get rid of these pseudo-private feature flags
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.
What would be the concern there, that there would be a dependency used in a contract being built that for some reason uses a __abi-embed feature flag?
Yup, that's the primary concern. Arguably, a good approach would be to localize effects instead of globalizing.
--all-features
causes cargo to activate all features in theCargo.toml
file.This patch introduces an intermediary flag. Unknown to cargo, but injected with a build script if;
__abi-embed
feature flag is activated.CARGO_NEAR_ABI_PATH
env is defined.This way, it's never implicitly activated. Resolving the compilation error in #970.
This also means, we gravitate from this compilation error:
.. to this compilation warning ..