abigen! macro fails to read .json file on Windows

[JoshuaBatty]

I found a bug where the abigen! macro works for me when compiling on my mac, but fails when building on windows.

The error i'm getting is:

message: called `Result::unwrap()` on an `Err` value: unable to canonicalize file from working dir C:\Users\conta\Desktop\counter_project with path /C:/Users/conta/Desktop/counter_project/my_contract_abi.json

I'll take a look into this.

[JoshuaBatty]

Ok after some more digging it seems that the Url crate is where things are breaking.

Local device paths are not supported in the Url crate on Windows (see this issue)

If I rewrite the with_root function here to exclude any paths created using the Url crate and simply use rusts' path joining as below then everything runs correctly on UNIX systems and Windows.

 Ok(Source::local(root.as_ref().join(source.as_ref())))

@digorithm what are your thoughts on this? One option would be to remove the option to load an ABI from a URL entirely as it's not guaranteed that the URL exists. The other option which feels a bit hacky is to check if #[cfg(target_os = "windows")] and skip trying to create a PathBuf using Url and instead just use the above method.

Keen to hear your thoughts.

[mitchmindtree]

I think we should be very careful about our intentions to support URLs in the future (especially during proc macro expansion), and should consider avoiding it altogether.

Cargo works hard at ensuring builds are reproducible with Cargo.lock, but if we support fetching from arbitrary URLs during macro expansion we run the risk of allowing users to get into situations where their builds break easily because they happen to be offline, or because the contract at the URL happened to change slightly, etc

My inclination would be to remove url and code related to Url sources for now until we work out a safer / more reproducible-friendly way of supporting it.

---

Further ramblings about cargo

While cargo's support for arbitrary Rust during build scripts and macro expansion is very powerful, I think allowing for arbitrary I/O is widely considered a bit of a mistake. It makes reliable caching of builds nearly impossible and makes it incredibly difficult to ensure that Rust projects are truly reproducible without constraining the whole build environment using fancy shell tricks like Nix does.

I would like to have seen cargo add support for external inputs (like remote files) by declaring them in the manifest file similarly to how we already do for [dependencies] so that 1. they could be fetched while the rest of the dependencies are being fetched and 2. they can be pinned alongside the rest of the crate dependencies.

[digorithm]

Ok after some more digging it seems that the Url crate is where things are breaking.

Local device paths are not supported in the Url crate on Windows (see this issue)

If I rewrite the with_root function here to exclude any paths created using the Url crate and simply use rusts' path joining as below then everything runs correctly on UNIX systems and Windows.

 Ok(Source::local(root.as_ref().join(source.as_ref())))

@digorithm what are your thoughts on this? One option would be to remove the option to load an ABI from a URL entirely as it's not guaranteed that the URL exists. The other option which feels a bit hacky is to check if #[cfg(target_os = "windows")] and skip trying to create a PathBuf using Url and instead just use the above method.

Keen to hear your thoughts.

First option sounds really good. I'm all for not supporting URLs. Like you both said, lots of potential issues when handling URLs in this case (e.g. in macro expansion).

The original rationale of supporting URLs was just to keep a light feature parity with ethers-rs. But this seems more trouble than what it is worth. So feel free to simplify; supporting more platforms > supporting URLs, IMO.