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

Refactor redirect_stderr_to_file to solana-logger #64

Merged
merged 6 commits into from
Mar 5, 2025

Conversation

lijunwangs
Copy link
Contributor

@lijunwangs lijunwangs commented Mar 3, 2025

redirect_stderr_to_file can be used multiple applications. I found the need to use it in vortexor. Moving this from validator to solana-logger.

Refactored from https://github.com/anza-xyz/agave/blob/master/validator/src/lib.rs#L39 which will be removed once this change made to the SDK

@alexpyattaev
Copy link
Contributor

Looks good, this code was stuck in validator crate but can be useful to every CLI tool that uses solana_logger crate.

Copy link
Collaborator

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Since solana-logger already transitively depends on libc, the new signal-hook dependency adds very little to the build time, around 0.1s on my machine, so it looks good to me!

Just some tiny comments.

@@ -2,7 +2,7 @@
name = "solana-logger"
description = "Solana Logger"
documentation = "https://docs.rs/solana-logger"
version = "2.2.1"
version = "2.2.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You shouldn't do the version bump here, the release job will take care of that automatically as mentioned at https://github.com/anza-xyz/solana-sdk?tab=readme-ov-file#publishing-a-crate-from-this-repository

Comment on lines 83 to 84
#[cfg(unix)]
#[cfg(not(target_arch = "wasm32"))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: these can get condensed into one

Suggested change
#[cfg(unix)]
#[cfg(not(target_arch = "wasm32"))]
#[cfg(all(unix, not(target_arch = "wasm32")))]

Copy link
Collaborator

@joncinque joncinque left a 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!

@lijunwangs lijunwangs merged commit 54c86b1 into master Mar 5, 2025
20 checks passed
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.

3 participants