-
Notifications
You must be signed in to change notification settings - Fork 450
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
stylus: add compilation target config, support multiple targets in wasm store #2463
Conversation
also introduce compilation target
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small request for a comment but other than that LGTM
@@ -56,17 +55,21 @@ pub fn target_cache_set(name: String, description: String, native: bool) -> Resu | |||
} | |||
return Err(eyre!(err_message)); | |||
} | |||
InitCache::set_target(target.clone()) | |||
*TARGET_NATIVE.write() = target.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still add a comment to target_cache_set
explaining everything this function does, because it kind of does two things in one. It both populates TARGET_CACHE, but if native is set, it also populates TARGET_NATIVE which isn't really a cache or related to TARGET_CACHE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a following comment:
/// Populates `TARGET_CACHE` inserting target specified by `description` under `name` key.
/// Additionally, if `native` is set it sets `TARGET_NATIVE` to the specified target.
Let me know if that's good / precise enough :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The docker build seems to be failing with
and it's not failing on master https://github.com/OffchainLabs/nitro/actions/runs/10423011222/job/28868789568 Did we accidentally introduce a new wasmer-go dependency? |
It looks like it's from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but now has conflicts with master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Resolves: NIT-2631
Resolves: NIT-2695
This PR:
Based on: #2417
Pulls: OffchainLabs/go-ethereum#339