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

Update clap to v3.1.5 for all the repository #23557

Closed
KirillLykov opened this issue Mar 9, 2022 · 14 comments
Closed

Update clap to v3.1.5 for all the repository #23557

KirillLykov opened this issue Mar 9, 2022 · 14 comments
Assignees
Labels
dependencies stale [bot only] Added to stale content; results in auto-close after a week.

Comments

@KirillLykov
Copy link
Contributor

KirillLykov commented Mar 9, 2022

Problem

Most of the targets use clap v2.33, we want to upgrade to v3.1.5

Proposed Solution

Prepare several PRs to address the problem:

  1. Update clap for all bpf utils #23555
  2. Update clap for net-sharper, net-utils, log-analyzer, bench-streamer, poh-bench #23556
  3. Add clap-v3-utils #24096
  4. Clap v3 update: keygen #24479
  5. Clap3 update: cli #24594
  6. Other changes are in the Clap3 update: all that depend on clap-utils #23864. The idea is to split this PR into several smaller PR (see discussion below)
@KirillLykov KirillLykov self-assigned this Mar 9, 2022
@KirillLykov
Copy link
Contributor Author

KirillLykov commented Mar 14, 2022

Problem with (2):

  1. When building all the downstream projects (./scripts/build-downstream-projects.sh), fails with errors inside serum/dex related to clap.
  2. This happens because serum uses unstable version 3.0.0-beta.1. Hence, a greater minor version picked up (3.1.6). This happens because assumption is that API is stable within one major version, this assumption is broken in this case because serum uses unstable beta version
  3. Just changing cargo file on serum side leads to problem with conflicting packages version (with quote).

Action items:

  1. Open issue on the serum side with explanation: Update clap to v3.1.6: clap version used in dex has unstable API project-serum/serum-dex#221
  2. Try to fix it quickly
  3. if this attempts fails, propose a PR with clap-related changes and ask serum to do the rest cargo work

@KirillLykov KirillLykov changed the title Update clap to v3.15 for all the repository Update clap to v3.1.5 for all the repository Mar 14, 2022
@KirillLykov
Copy link
Contributor Author

KirillLykov commented Mar 21, 2022

Vim commands to update some clap calls:

%s/Arg::with_name/Arg::new/g
%s/value_t!(matches, /matches.value_of_t(/g
%s/value_t_or_exit!(matches, /matches.value_of_t_or_exit(/g
%s/value_t!(matches, /matches.value_of_t(/g
%s/short("\([^']*\)")/short('\1')/g
%s/.validator(\([^']*\))/.validator(|s| \1(s))/g 
%s/.multiple(true)/.multiple_occurrences(true).multiple_values(true)/g
%s/.setting(AppSettings::DisableVersion)/.disable_version_flag(true)/g
%s/.hidden(true)/.hide(true)/g

@mvines
Copy link
Contributor

mvines commented Mar 22, 2022

Hey have you considered how to handle the helper functions in the clap-utils crate? Examples include input_validators::is_parsable() and input_parsers::pubkey_of(). We can't simply flip these over to use clap 3 as that would break existing downstream users of the clap 2 functions.

One possible approach is to create a solana_clap_utils::v3 module for the v3 versions and leave the v2 versions as is

@KirillLykov
Copy link
Contributor Author

Hey have you considered how to handle the helper functions in the clap-utils crate? Examples include input_validators::is_parsable() and input_parsers::pubkey_of(). We can't simply flip these over to use clap 3 as that would break existing downstream users of the clap 2 functions.

One possible approach is to create a solana_clap_utils::v3 module for the v3 versions and leave the v2 versions as is

By downstream users, do you mean those mentioned in build-downstream-projects.sh or there are some which are probably not known to us and we cannot break them?

With clap-utils it is hard by itself to do the upgrade because many other tools within our repo depends on it and some clap's API changes are very particular. It mean that it might be a rather big PR.

Having two versions of clap in one repo is probably possible but I found it difficult due to usage of attributes. Haven't figured out yet how to overcome these difficulties.

@mvines
Copy link
Contributor

mvines commented Mar 22, 2022

By downstream users, do you mean those mentioned in build-downstream-projects.sh or there are some which are probably not known to us and we cannot break them?

That's a start but there are other cli users of solana-clap-utils that aren't in that minimal ci script. I played with it a little and I think a new solana-clap-utils-v3 crate (clap-utils-v3/ in the monorepo) that's literally just a duplicate of what's in clap-utils/ but modified for clapv3 is the way to go. This allows downstream users to opt into clapv3 at the time of their choosing, and we're under no obligation to build more features into clap-utils/ as we add them into clap-utils-v3/ going forward

@KirillLykov
Copy link
Contributor Author

By downstream users, do you mean those mentioned in build-downstream-projects.sh or there are some which are probably not known to us and we cannot break them?

That's a start but there are other cli users of solana-clap-utils that aren't in that minimal ci script. I played with it a little and I think a new solana-clap-utils-v3 crate (clap-utils-v3/ in the monorepo) that's literally just a duplicate of what's in clap-utils/ but modified for clapv3 is the way to go. This allows downstream users to opt into clapv3 at the time of their choosing, and we're under no obligation to build more features into clap-utils/ as we add them into clap-utils-v3/ going forward

To verify that I understood it correctly:

  • add clap-utils-v3 which will coexist with clap-utils (clap v2)
  • all the crates inside solana repo will eventually depend on clap-utils-v3
  • external crates might continue using clap-utils and migrate to clap-utils-v3 when they are ready to do so. Probably, a good idea is to add a notice about this in the crate.

On the operational side I like this idea because this will also allow to have smaller PRs

@KirillLykov
Copy link
Contributor Author

This PR which migrates the heaviest part of the code base was automatically closed: #24594 (comment)
For the case we want to come back to this migration later

@gitmalong
Copy link

clap v2.34.0
├── solana-clap-utils v1.14.5 ()
├── solana-client v1.14.5 (
)
└── solana-faucet v1.14.5 (*)

clap v3.2.22
└── solana-net-utils v1.14.5 (*)

@KirillLykov
Copy link
Contributor Author

clap v2.34.0 ├── solana-clap-utils v1.14.5 () ├── solana-client v1.14.5 () └── solana-faucet v1.14.5 (*)

clap v3.2.22 └── solana-net-utils v1.14.5 (*)

Sorry, but I don't quite understand what do you want to say by this message

@gitmalong
Copy link

clap v2.34.0 ├── solana-clap-utils v1.14.5 () ├── solana-client v1.14.5 () └── solana-faucet v1.14.5 ()
clap v3.2.22 └── solana-net-utils v1.14.5 (
)

Sorry, but I don't quite understand what do you want to say by this message

I wanted to point out that there still clap v2 dependencies which leads to several clap versions in the dependency tree if one relies on those Solana packages.

@KirillLykov
Copy link
Contributor Author

KirillLykov commented Oct 21, 2022

@gitmalong that's a known thing, migration from clap v2 to v3 turned out to be too risky and was postponed

@KirillLykov
Copy link
Contributor Author

KirillLykov commented Mar 15, 2023

Check: can we avoid panicking by switching to try_get_matches?
another relevant suggestion: https://discord.com/channels/428295358100013066/439194979856809985/1085599192732401746

@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Mar 15, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 22, 2024
@KirillLykov
Copy link
Contributor Author

I think it is still active, @samkim-crypto is doing this migration

@KirillLykov KirillLykov reopened this Mar 22, 2024
Copy link
Contributor

This repository is no longer in use. Please re-open this issue in the agave repo: https://github.com/anza-xyz/agave

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependencies stale [bot only] Added to stale content; results in auto-close after a week.
Projects
None yet
Development

No branches or pull requests

3 participants