near / borsh-rs

Rust implementation of Binary Object Representation Serializer for Hashing
https://borsh.io/
Apache License 2.0
306 stars 67 forks source link

`derive` macros require `cargo` to be present #271

Closed Fuuzetsu closed 9 months ago

Fuuzetsu commented 10 months ago

We use a build system where cargo is not involved during the build (via https://github.com/NixOS/nixpkgs/)

However when building a dependency with borsh-derive, we get the below issue. looking at check_attrs_get_cratename I can trace it to crate_name from proc_macro, notably workspace_manifest_path which seems to want to use whatever binary in CARGO env variable to then get the manifest path.

It appears the whole point is to get the borsh crate name as the user has set it to then generate trait impls like impl … #cratename::de::BorshDeserialize for … {.

Probably we should be doing whatever serde does that doesn't involve cargo without having to give up on having a separate derive crate (of course if it was just the same crate, I think just using $crate would have worked).

I might try to cook up a patch today.

rust_bit-vec> error: proc-macro derive panicked
rust_bit-vec>    --> src/lib.rs:219:12
rust_bit-vec>     |
rust_bit-vec> 219 |     derive(borsh::BorshDeserialize, borsh::BorshSerialize)
rust_bit-vec>     |            ^^^^^^^^^^^^^^^^^^^^^^^
rust_bit-vec>     |
rust_bit-vec>     = help: message: called `Result::unwrap()` on an `Err` value: `CARGO` env variable not set.
rust_bit-vec> thread '<unnamed>' panicked at src/internals/cratename.rs:21:35:
rust_bit-vec> called `Result::unwrap()` on an `Err` value: `CARGO` env variable not set.
rust_bit-vec> stack backtrace:
rust_bit-vec>    0: rust_begin_unwind
rust_bit-vec>    1: core::panicking::panic_fmt
rust_bit-vec>    2: core::result::unwrap_failed
rust_bit-vec>    3: borsh_derive::check_attrs_get_cratename
rust_bit-vec>    4: borsh_derive::borsh_serialize
rust_bit-vec>    5: proc_macro::bridge::selfless_reify::reify_to_extern_c_fn_hrt_bridge::wrapper
rust_bit-vec>    6: <proc_macro::bridge::server::MaybeCrossThread<rustc_expand::proc_macro::CrossbeamMessagePipe<proc_macro::bridge::buffer::Buffer>> as proc_macro::bridge::server::ExecutionStrategy>::run_bridge_and_client::<proc_macro::bridge::server::Dispatcher<proc_macro::bridge::server::MarkedTypes<rustc_expand::proc_macro_server::Rustc>>>
rust_bit-vec>    7: proc_macro::bridge::server::run_server::<rustc_expand::proc_macro_server::Rustc, proc_macro::bridge::Marked<rustc_ast::tokenstream::TokenStream, proc_macro::bridge::client::TokenStream>, core::option::Option<proc_macro::bridge::Marked<rustc_ast::tokenstream::TokenStream, proc_macro::bridge::client::TokenStream>>, proc_macro::bridge::server::MaybeCrossThread<rustc_expand::proc_macro::CrossbeamMessagePipe<proc_macro::bridge::buffer::Buffer>>>
rust_bit-vec>    8: <rustc_expand::proc_macro::DeriveProcMacro as rustc_expand::base::MultiItemModifier>::expand
rust_bit-vec>    9: <rustc_expand::expand::MacroExpander>::fully_expand_fragment
rust_bit-vec>   10: <rustc_expand::expand::MacroExpander>::expand_crate
rust_bit-vec>   11: <rustc_session::session::Session>::time::<rustc_ast::ast::Crate, rustc_interface::passes::configure_and_expand::{closure#1}>
rust_bit-vec>   12: rustc_interface::passes::resolver_for_lowering
rust_bit-vec>       [... omitted 2 frames ...]
Fuuzetsu commented 10 months ago

After surveying some popular crates, it seems that the general approach is to expose some crate =attribute that allows specifying the name explicitly: borsh already supports this I believe. Without it, just breaks if rename happens.

Only a few crates use proc-macro-crate helper that goes via cargo, especially because it makes the macro unpure (goes via filesystem...).

There doesn't seem to be a good solution other than these options...

dj8yfo commented 9 months ago

@Fuuzetsu looking up nix doc https://nixos.org/manual/nixpkgs/stable/#compiling-rust-crates-using-nix-instead-of-cargo, it looks like giving up cargo completely is a viable option.

272 is one of possible ways to resolve this, if adding #[borsh(crate = "borsh")] everywhere, effectively disabling proc-macro-crate checks, isn't too ergonomic.

Fuuzetsu commented 9 months ago

@Fuuzetsu looking up nix doc https://nixos.org/manual/nixpkgs/stable/#compiling-rust-crates-using-nix-instead-of-cargo, it looks like giving up cargo completely is a viable option.

Hm? Not sure what you mean by this but right, we already do not use cargo during the build.

272 is one of possible ways to resolve this, if adding #[borsh(crate = "borsh")] everywhere, effectively disabling proc-macro-crate checks, isn't too ergonomic.

Right, making it optional is something I guess. Presumably in cases where one renamed borsh and has a very large number of derives.

FWIW for now we're just using a fork with the proc-macro-crate deleted from the repo all together...

frol commented 9 months ago

@dj8yfo I don't think we need to introduce a breaking change to borsh-rs to accommodate non-standard use of build systems.

@Fuuzetsu Please, consider contributing to proc-macro-crate so it just supports your build environment.

P.S. I closed this issue to indicate that we don't plan to take any actions on it now as it is outside of the scope of this crate, but if you believe otherwise, please, feel free to comment below