solana icon indicating copy to clipboard operation
solana copied to clipboard

Update clap to v3.1.5 for all the repository

Open KirillLykov opened this issue 3 years ago • 12 comments

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. https://github.com/solana-labs/solana/pull/23555
  2. https://github.com/solana-labs/solana/pull/23556
  3. https://github.com/solana-labs/solana/pull/24096
  4. https://github.com/solana-labs/solana/pull/24479
  5. https://github.com/solana-labs/solana/pull/24594
  6. Other changes are in the https://github.com/solana-labs/solana/pull/23864. The idea is to split this PR into several smaller PR (see discussion below)

KirillLykov avatar Mar 09 '22 14:03 KirillLykov

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: https://github.com/project-serum/serum-dex/issues/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 avatar Mar 14 '22 13:03 KirillLykov

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

KirillLykov avatar Mar 21 '22 17:03 KirillLykov

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

mvines avatar Mar 22 '22 04:03 mvines

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.

KirillLykov avatar Mar 22 '22 09:03 KirillLykov

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

mvines avatar Mar 22 '22 15:03 mvines

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 avatar Mar 22 '22 15:03 KirillLykov

This PR which migrates the heaviest part of the code base was automatically closed: https://github.com/solana-labs/solana/pull/24594#issuecomment-1179810171 For the case we want to come back to this migration later

KirillLykov avatar Jul 11 '22 11:07 KirillLykov

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 (*)

gitmalong avatar Oct 18 '22 19:10 gitmalong

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

KirillLykov avatar Oct 19 '22 11:10 KirillLykov

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.

gitmalong avatar Oct 21 '22 12:10 gitmalong

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

KirillLykov avatar Oct 21 '22 14:10 KirillLykov

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

KirillLykov avatar Mar 15 '23 16:03 KirillLykov

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

KirillLykov avatar Mar 22 '24 08:03 KirillLykov

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

github-actions[bot] avatar Mar 22 '24 08:03 github-actions[bot]