Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Refactor, move solana-validator CLI related configs to its own module#28548

Merged
mvines merged 4 commits intosolana-labs:masterfrom
eloylp:refactor-validator-main
Nov 19, 2022
Merged

Refactor, move solana-validator CLI related configs to its own module#28548
mvines merged 4 commits intosolana-labs:masterfrom
eloylp:refactor-validator-main

Conversation

@eloylp
Copy link
Copy Markdown
Contributor

@eloylp eloylp commented Oct 22, 2022

Problem

The main function of the validator is getting big. I found it a bit difficult to track the code.

Summary of Changes

There is no behavioural change on this PR. Its just a refactor:

  • Move the CLI initialisation code and validations to their own module, cli.rs, leaving the main function a bit more lightweight.
  • Group all the default arguments in a dedicated struct. So easier to find and alter them.

Checks that i did

The most sensible change here are the defaults grouping. As the CLI defaults are shown in the output when invoking it with --help, i thought the following commands to be useful for a minimal check, to ensure no behavioural change:

cd solana/validator
## Save CLI output with defaults in master
$ git checkout master
$ cargo run -- --help > /tmp/master-cli-output.txt
## Save CLI output with defaults in the refactor branch
$ git checkout refactor-validator-main
$ cargo run -- --help > /tmp/refactor-cli-output.txt
## Compare md5 sums 
$ md5sum /tmp/master-cli-output.txt /tmp/refactor-cli-output.txt
12768cba6bb80d63e77b52fe162657d5  /tmp/master-cli-output.txt
12768cba6bb80d63e77b52fe162657d5  /tmp/refactor-cli-output.txt

And similar for the test validator part:

$ cd solana/validator

$ git checkout master
$ ./solana-test-validator --help > /tmp/master-test-cli-output.txt

$ git checkout refactor-validator-main
$ ./solana-test-validator --help > /tmp/refactor-test-cli-output.txt

$ md5sum /tmp/master-test-cli-output.txt /tmp/refactor-test-cli-output.txt 
c75c7b2a81c23961f68929838d07d5b9  /tmp/master-test-cli-output.txt
c75c7b2a81c23961f68929838d07d5b9  /tmp/refactor-test-cli-output.txt

Edit: updated with latest changes (conflict solving).

Edit2: updated with latest changes (conflict solving). Added test validator check.

@mergify mergify Bot added the community Community contribution label Oct 22, 2022
@mergify mergify Bot requested a review from a team October 22, 2022 15:26
@eloylp eloylp marked this pull request as draft November 4, 2022 21:57
@eloylp eloylp force-pushed the refactor-validator-main branch from da112f4 to b84aead Compare November 4, 2022 22:23
@eloylp eloylp marked this pull request as ready for review November 4, 2022 22:27
@eloylp
Copy link
Copy Markdown
Contributor Author

eloylp commented Nov 4, 2022

Solved conflicts. Looks like 2 arguments related to blockstore were deprecated at #28409 . Ready for review !

@eloylp
Copy link
Copy Markdown
Contributor Author

eloylp commented Nov 11, 2022

Hi @mvines , apologies for the direct ping, just trying to follow contributing guides . Could you help me on reviewing this ? Thank you in advance !

@eloylp
Copy link
Copy Markdown
Contributor Author

eloylp commented Nov 11, 2022

Ouups, just realised it got conflicts again. Will move it to draft again. Early feedback still appreciated.

@eloylp eloylp marked this pull request as draft November 11, 2022 21:04
@eloylp eloylp force-pushed the refactor-validator-main branch 3 times, most recently from 97de5fd to ed7c774 Compare November 12, 2022 00:56
@eloylp
Copy link
Copy Markdown
Contributor Author

eloylp commented Nov 12, 2022

The branch is now updated with latest changes. The last conflict was due to this new addition . I think this is ready for review again.

@eloylp eloylp marked this pull request as ready for review November 12, 2022 01:00
@mvines mvines added the CI Pull Request is ready to enter CI label Nov 18, 2022
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Nov 18, 2022
Copy link
Copy Markdown
Contributor

@mvines mvines left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. Looks fine to me overall, just one nit.

Do you think validator/src/bin/solana-test-validator.rs deserves the same treatment for its cli config?

.long("no-bpf-jit")
.takes_value(false)
.help("Disable the just-in-time compiler and instead use the interpreter for SBF. Windows always disables JIT."),
.help("Disable the just-in-time compiler and instead use the interpreter for BPF. Windows always disables JIT."),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oops can you please restore this line

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed at ae6f93f

@mvines
Copy link
Copy Markdown
Contributor

mvines commented Nov 18, 2022

Looks like CI wants you to run cargo fmt :)

The main function of the validator is getting big. Improve readability by moving away the CLI setup stuff to its own module.
@eloylp eloylp force-pushed the refactor-validator-main branch from ed7c774 to f31988c Compare November 18, 2022 23:56
@eloylp
Copy link
Copy Markdown
Contributor Author

eloylp commented Nov 19, 2022

Hello @mvines. Here is a list of updates:

  • Do you think validator/src/bin/solana-test-validator.rs deserves the same treatment for its cli config?

Yes. I was worried about the size of the PR. Planned to do it a second PR. But here is it f31988c . Let me know what do you think. I did the same md5 checks as in the first refactor (see updated PR description).

  • Updated with latest changes from master. A conflict was fixed, due to this PR . I did again the md5 checks (see PR description).

  • Configured cargo fmt "on save" on my new setup. It should not fail again :)

Probably a squash is a good idea if merged. I think this is ready for review again !

@mvines mvines added the CI Pull Request is ready to enter CI label Nov 19, 2022
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Nov 19, 2022
@eloylp
Copy link
Copy Markdown
Contributor Author

eloylp commented Nov 19, 2022

Ah ! gotcha. CI failed again. Thats probably because:

$ cargo fmt --all
Warning: can't set `imports_granularity = One`, unstable features are only available in nightly channel.
...

This one did the trick:

cargo +nightly fmt --all

Fixed at 5b05edc . I hope. Sorry inconveniences.

@mvines mvines added the CI Pull Request is ready to enter CI label Nov 19, 2022
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Nov 19, 2022
@mvines mvines merged commit 3d91c3f into solana-labs:master Nov 19, 2022
@mvines
Copy link
Copy Markdown
Contributor

mvines commented Nov 19, 2022

Thanks for the PR!

@eloylp
Copy link
Copy Markdown
Contributor Author

eloylp commented Nov 19, 2022

Thanks for the PR!

Thanks for review 🙂

gnapoli23 pushed a commit to gnapoli23/solana that referenced this pull request Dec 16, 2022
…solana-labs#28548)

* Refactor, move validator CLI related stuff to its own module(cli.rs)

The main function of the validator is getting big. Improve readability by moving away the CLI setup stuff to its own module.

* Restore help cli line

* Refactor,  move test validator CLI config to cli.rs module

* Fix  imports (cargo fmt)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

community Community contribution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants