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

No std support? #98

Closed
cbeck88 opened this issue Jan 8, 2020 · 6 comments
Closed

No std support? #98

cbeck88 opened this issue Jan 8, 2020 · 6 comments

Comments

@cbeck88
Copy link

cbeck88 commented Jan 8, 2020

Hi, this issue may sound a bit strange, let me explain:

  • We have many servers which use SGX enclaves. For us, it is natural that the rust package corresponding server which requires an enclave, should ensure that the enclave gets built. But it is not a natural rust dependency, the enclave is compiled as a shared library which is then loaded into the CPU via Intel APIs, and it has special requirements like, it cannot link to libstd.
  • This means that we have server packages, whose build.rs shells out to cargo in order to build the enclave package. That invocation takes place in a different cargo workspace.
  • In order to get correct incremental builds, we want to emit cargo:rerun-if-changed directives from that build.rs, corresponding to any local packages depended on by the enclave.
  • In order to get those paths, we want to use cargo_metadata to find any local packages depended on by the enclave, which works :)
  • Unfortunately, because cargo_metadata depends on serde_json, it brings the std feature into serde and then breaks the enclave build. This feature unification across dependencies and build-dependencies is a long-standing cargo bug, with no fix in sight: build-dependencies and dependencies should not have features unified rust-lang/cargo#4866

After some deliberation, what I have done now is clone cargo_metadata and republish it on crates.io as alt_cargo_metadata. I have also republished alt_serde and alt_serde_json, and patched their Cargo.toml's to depend on alt versions of each other. Cargo allows renaming dependencies via package = so the code still refers to serde, but Cargo does not unify features between serde and alt-serde, so our build can limp along.

What I'd like to ask is, would you consider accepting a patch that migrates cargo_metadata away from serde_json towards something that does not use std, and specifically, doesn't require serde/std? It seems like a design flaw that serde_json cannot be used at all without serde/std, it seriously limits its usefulness.

I'd also be interested if you think there's a better way to track dependencies of a cargo invocation made from a build.rs script than using this package -- I guess that you are mainly interested in supporting cargo subcommand tools, so maybe this use is out of scope?

Thanks!

@oli-obk
Copy link
Owner

oli-obk commented Jan 8, 2020

I've pondered on this a bit and can't really come up with a good solution. Moving away from serde is a non-starter for me.

This means that we have server packages, whose build.rs shells out to cargo in order to build the enclave package.

If you are shelling out anyway, you could create a binary crate that when run, runs your cargo_metadata logic (including the println! with the cargo:rerun-if-changed). Since it would be a completely separate crate with its own Cargo.toml, you wouldn't end up with this conflict.

Basically move your build.rs contents to a separate binary crate and make your build.rs just a shim to invoke cargo run on that separate binary crate.

@cbeck88
Copy link
Author

cbeck88 commented Jan 8, 2020

I like your proposal, it sounds promising. Thank you!

@cbeck88 cbeck88 closed this as completed Jan 8, 2020
@cbeck88
Copy link
Author

cbeck88 commented Jan 22, 2020

Hi, just fyi, there was a recent PR to serde/json that supports no_std configuration:

serde-rs/json#606

So another possibility is to select json with the alloc feature instead of std in your cargo toml. It might work without any source changes after that, because I doubt you are doing anything with serde/json that requires the full std.

In our build we resolved in a different way so I don't have a patch to offer, but if someone else has this problem and sees this issue, it might help them

@oli-obk oli-obk reopened this Mar 1, 2021
@jyn514
Copy link
Collaborator

jyn514 commented Apr 7, 2021

This is not currently possible because cargo_metadata exposes std types in the API:

So I'm not sure this makes sense to keep open.

@cbeck88
Copy link
Author

cbeck88 commented Apr 9, 2021

this was basically fixed by cargo adding the new resolver, which allows to avoid unifying build dependencies and source dependencies. so fwiw if you want to close it, that seems reasonable

@jyn514
Copy link
Collaborator

jyn514 commented Apr 9, 2021

this was basically fixed by cargo adding the new resolver, which allows to avoid unifying build dependencies and source dependencies. so fwiw if you want to close it, that seems reasonable

Ahhh, I missed that this was the issue - you don't need cargo_metadata itself to be no_std, just its dependencies. Yes, I think this makes sense to close then.

@jyn514 jyn514 closed this as completed Apr 9, 2021
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

No branches or pull requests

3 participants