Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

(discussion) Create custom rustc target for Substrate runtime #4225

Closed
tomusdrw opened this issue Nov 27, 2019 · 11 comments
Closed

(discussion) Create custom rustc target for Substrate runtime #4225

tomusdrw opened this issue Nov 27, 2019 · 11 comments
Labels
Z7-question Issue is a question. Closer should answer.

Comments

@tomusdrw
Copy link
Contributor

From convos with @tomaka :

The idea is that we'd create a JSON file named wasm32-substrate.json, containing a copy of the one of wasm32-unknown-unknown (there's a Rust command to print the built-in target definitions on stdout)
And then you do cargo +nightly build -Z build-std=alloc,core --target=wasm32-substrate
And everything should magically work
Then we could do #[cfg(target_os = "substrate")]
We could conditionally-compile code for substrate or non-substrate
Problematic part is that if people did #[cfg(target_os = "unknown")] then it won't work anymore

I don't have opinion on this topic, but would be good to have some discussion here. CC @bkchr @pepyakin

@tomusdrw tomusdrw added the Z7-question Issue is a question. Closer should answer. label Nov 27, 2019
@tomaka
Copy link
Contributor

tomaka commented Nov 27, 2019

Small note: there's a bug in Rust/Cargo right now, and you have to pass the RUST_TARGET_PATH environment variable set to the path that contains wasm32-substrate.json, otherwise the definition will not be found when compiling dependencies.

@tomusdrw
Copy link
Contributor Author

Just realized that afaict a nice consequence would be getting rid of all the std features in all crates and issues with it's propagation (i.e. something pulling in std, cause you forgot default-features = false). Basically we would just feature gate with #[cfg(target_os = "substrate")] instead.

@bkchr
Copy link
Member

bkchr commented Nov 28, 2019

Hmm, does this works with the rest of the rust ecosystem that is not controlled by us? They don't know about target_os = "substrate" and will probably not accept patches for such stuff.
We still would need to have default-features = false in our Cargo.toml's and AFAIK you can not enable/disable features based on your target.

@tomaka
Copy link
Contributor

tomaka commented Nov 28, 2019

Just realized that afaict a nice consequence would be getting rid of all the std features in all crates and issues with it's propagation (i.e. something pulling in std, cause you forgot default-features = false). Basically we would just feature gate with #[cfg(target_os = "substrate")] instead.

It does make sense to do that for code that calls externalities.
You know the externality exists because you're compiling for Substrate. Assuming that it is there because std is disabled is a just guess, and is less clean.

@tomusdrw
Copy link
Contributor Author

@bkchr wouldn't this work:

[target.'cfg(not(target_os = "substrate"))'.dependencies]
native = { path = "./a" }

[target.'cfg(target_os = "substrate")'.dependencies]
native = { path = "./a", default-features = false }

https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies

Or are features unified between platforms anyway?

@bkchr
Copy link
Member

bkchr commented Nov 28, 2019

Just realized that afaict a nice consequence would be getting rid of all the std features in all crates and issues with it's propagation (i.e. something pulling in std, cause you forgot default-features = false). Basically we would just feature gate with #[cfg(target_os = "substrate")] instead.

It does make sense to do that for code that calls externalities.
You know the externality exists because you're compiling for Substrate. Assuming that it is there because std is disabled is a just guess, and is less clean.

Good point :)

@tomusdrw IDK :)

@tomaka
Copy link
Contributor

tomaka commented Nov 28, 2019

@bkchr wouldn't this work:

I'm pretty sure that should work.

@bkchr
Copy link
Member

bkchr commented Nov 28, 2019

rust-lang/cargo#6121

@tomusdrw
Copy link
Contributor Author

Closing, since imho the biggest gain would be if we could avoid std features in our own crates, but it doesn't seem to be possible because of the cargo issue mentioned by Basti.

@tomaka
Copy link
Contributor

tomaka commented Dec 10, 2019

Why not leave this open as a long-term goal? It's strictly more correct than what we're doing.

@tomusdrw
Copy link
Contributor Author

@tomaka feel free to reopen, but I don't see that many benefits nor will to implement this at all (based on the "amount" of discussion here). Given that we have some ideas of issue tracker being a short-term, achievable tasks I feel it would be moved or closed anyway.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Z7-question Issue is a question. Closer should answer.
Projects
None yet
Development

No branches or pull requests

3 participants