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

Clean-up guest builds #62

Closed
wants to merge 5 commits into from
Closed

Conversation

capossele
Copy link
Contributor

@capossele capossele commented Nov 30, 2023

As the [target.'cfg(not(target_os = "zkvm"))'.dependencies] does not seem to work for some reason, this PR exposes the feature host (enabled by default) to prevent unnecessary deps ending up in the guest builds

@capossele capossele marked this pull request as ready for review November 30, 2023 11:53
Copy link
Contributor

@Wollac Wollac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the Cargo.lock it seems to work, but I do not understand the details:

  • The TOML still contains [target.'cfg(not(target_os = "zkvm"))'.dependencies]. So this is still needed, but simply adding the optional will do the trick?
  • Building the lib with --no-default-features now fails because the target OS is not zkvm, but all those dependencies are not included. This also causes problems with the rust-analyzer. Is there a way to fix this? One alternative I could think of is to remove all #[cfg(not(target_os = "zkvm"))] and manually enable/disable the host feature. However, I am not sure if this is a good idea.

lib/Cargo.toml Outdated Show resolved Hide resolved
@capossele
Copy link
Contributor Author

Yeah I'm not sure why [target.'cfg(not(target_os = "zkvm"))'.dependencies] does not really work as expected. It can be removed though if properly switching as you suggested from #[cfg(not(target_os = "zkvm"))] to #[cfg(feature = "host"))].
I've pushed a commit doing that.

Overall this is just a suggestion, maybe a bit hacky, so no harm in closing it if you guys are not convinced. Obviously would be better to understand why [target.'cfg(not(target_os = "zkvm"))'.dependencies] does not properly work

@capossele
Copy link
Contributor Author

Actually I did not feel comfortable at removing the #[cfg(not(target_os = "zkvm"))]. So I ended up having them both

@Wollac
Copy link
Contributor

Wollac commented Dec 12, 2023

It seems that [target.'cfg(not(target_os = "zkvm"))'.dependencies] actually works as expected, but the disabled dependencies show up in the Cargo.lock of the guest. However, they are resolved correctly. See: rust-lang/cargo#10801

@Wollac Wollac closed this Dec 12, 2023
@Wollac Wollac deleted the capossele/clean-up-guest-builds branch February 18, 2024 20:59
johntaiko added a commit to johntaiko/zeth that referenced this pull request Mar 22, 2024
* Refactor MptNode struct and methods

* fix: avoid RefCell cross-await
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

Successfully merging this pull request may close these issues.

2 participants