-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(anvil): added js tracer #11052
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
feat(anvil): added js tracer #11052
Conversation
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.
cool
@DaniPopes this introduces boa into the anvil dep graph, should we also feature gate this or is this fine?
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, thank you, left one nit.
Also this adds bunch of new deps, which I think it's fine @DaniPopes could you pls chime in?
Let's feature gate like we do for aws: disabled by default but enabled in makefile/release ci |
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.
then we also need to activate this for release builds here:
Line 22 in 391f37f
source $HOME/.profile && cargo build --release --features cast/aws-kms,cast/gcp-kms,forge/aws-kms,forge/gcp-kms \ |
like anvil/js-tracer
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!
@grandizzy do we also need to add this feature here: Lines 49 to 55 in 391f37f
|
yeah, unfortunately features are now specified in multiple places, needs to be updated here for non docker build as well https://github.com/foundry-rs/foundry/blob/master/.github/workflows/release.yml#L162 |
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!
this has a patched dep..now the deps match :)closes #10977
Closes #6764