-
Notifications
You must be signed in to change notification settings - Fork 148
Remove frozen-abi-macro dependency on solana-logger #400
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
Conversation
|
Hmm ok, CI failed because of this line: solana-sdk/frozen-abi-macro/src/lib.rs Line 290 in 1ad7ca9
Trying to figure out if we need this or not Edit: I have updated the PR to remove the use of |
7a6a988 to
2dbbd29
Compare
f14c26c to
b1fab5c
Compare
apfitzge
left a comment
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.
seems reasonable to print instead of log. logger should 100% be agave-logger not solana-logger
|
Hmm, I just noticed there are some log invocations in solana-sdk/frozen-abi/src/abi_digester.rs Line 151 in 1ad7ca9
These are basically unprintable now if we do not setup a logger; we could either setup |
|
I'd say go with |
b1fab5c to
13ac095
Compare
There are several log!() lines as well as the solana_logger::setup() in frozen-abi-macro. Adjust the log lines to println!() and eprintln!() to remove the dependency; these output lines are only in frozen-abi tests
47a68db to
0c9680e
Compare
joncinque
left a comment
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.
Just one comment on the nits bit
joncinque
left a comment
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.
Looks great to me!
A couple items are logged, and we can simply use eprintln!() instead.
This is part of a larger initiative to remove
solana-loggerfrom the SDK as it seemingly belongs with Agave