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

Moving Optimism bin to Optimism Cli #9439

Merged
merged 39 commits into from
Jul 29, 2024

Conversation

loocapro
Copy link
Contributor

Closes #9408

@loocapro loocapro marked this pull request as ready for review July 11, 2024 08:45
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

great!
a few more nits, then this is g2g
I'll need to go over a few things before we can merge this, because we need to update a few scripts that build the op-reth binary

@loocapro loocapro requested a review from mattsse July 11, 2024 09:52
Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

Comment on lines -13 to -16
#[cfg(not(feature = "optimism"))]
compile_error!("Cannot build the `op-reth` binary with the `optimism` feature flag disabled. Did you mean to build `reth`?");

#[cfg(feature = "optimism")]
Copy link
Member

Choose a reason for hiding this comment

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

lfg

Comment on lines 104 to 158
/// # Example
///
/// ```no_run
/// use reth::cli::Cli;
/// use reth_node_optimism::OptimismNode;
///
/// Cli::parse_args()
/// .run(|builder, _| async move {
/// let handle = builder.launch_node(OptimismNode::default()).await?;
///
/// handle.wait_for_node_exit().await
/// })
/// .unwrap();
/// ```
///
/// # Example
///
/// Parse additional CLI arguments for the node command and use it to configure the node.
///
/// ```no_run
/// use clap::Parser;
/// use reth::cli::Cli;
///
/// #[derive(Debug, Parser)]
/// pub struct MyArgs {
/// pub enable: bool,
/// }
///
/// Cli::parse()
/// .run(|builder, my_args: MyArgs| async move {
/// // launch the node
///
/// Ok(())
/// })
/// .unwrap();
/// ````
Copy link
Member

Choose a reason for hiding this comment

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

maybe out of scope, but I would prefer these to be executable examples. easy to keep up to date with potential type name changes then using cargo test --doc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we can finally do this now actually

Copy link
Member

Choose a reason for hiding this comment

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

either it's in scope and this pr fixes it, or open an issue pls

@mattsse mattsse added A-cli Related to the reth CLI A-op-reth Related to Optimism and op-reth labels Jul 11, 2024
@loocapro loocapro requested a review from emhane July 12, 2024 12:31
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

tysm!

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

just checked, we need to do a few more things before we can finalize this

TODO

@loocapro loocapro requested a review from mattsse July 12, 2024 13:17
@loocapro loocapro requested a review from DaniPopes as a code owner July 12, 2024 13:44
emhane
emhane previously requested changes Jul 13, 2024
Copy link
Member

Choose a reason for hiding this comment

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

the code that has been deleted from bin/reth, needs to move to reth-optimism-cli. so this requires having a debug command in reth-optimism-cli.

@emhane
Copy link
Member

emhane commented Jul 14, 2024

how's it going with this? need some help? I have a PR that's a dependent on this @loocapro

@loocapro
Copy link
Contributor Author

how's it going with this? need some help? I have a PR that's a dependent on this @loocapro

Hi @emhane, since i removed some cfg flags, I am running in some workspace dependency issue here:

https://github.com/paradigmxyz/reth/actions/runs/9909282475/job/27376994891?pr=9439

Any advice?

Comment on lines +11 to +16
[dependencies]
reth-node-builder.workspace = true
reth-cli-util.workspace = true
reth-optimism-cli.workspace = true
reth-tracing.workspace = true
reth-node-optimism.workspace = true
Copy link
Member

Choose a reason for hiding this comment

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

in response to #9439 (comment):

you will have to add an optimism feature to this manifest, and then look through these deps, and see which has an optimism feature...so they can be included, just like for the other optimism crates - eventually we hope to get rid off this!

@joshieDo
Copy link
Collaborator

release builds work fine: https://github.com/paradigmxyz/reth/actions/runs/10144116336/job/28046941974?pr=9439

@mattsse mattsse enabled auto-merge July 29, 2024 15:06
@loocapro
Copy link
Contributor Author

release builds work fine: https://github.com/paradigmxyz/reth/actions/runs/10144116336/job/28046941974?pr=9439

Thanks @joshieDo for taking this on!! Amazing work!!

@joshieDo joshieDo requested a review from emhane July 29, 2024 15:23
@mattsse mattsse added this pull request to the merge queue Jul 29, 2024
Merged via the queue into paradigmxyz:main with commit 56b2798 Jul 29, 2024
34 checks passed
mattsse added a commit that referenced this pull request Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Related to the reth CLI A-op-reth Related to Optimism and op-reth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add optimism binary in optimism-cli
4 participants