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

*: drop go 1.18 support, add go 1.21 support #3157

Merged
merged 11 commits into from
Oct 12, 2023
Merged

*: drop go 1.18 support, add go 1.21 support #3157

merged 11 commits into from
Oct 12, 2023

Conversation

AnnaShaleva
Copy link
Member

@AnnaShaleva AnnaShaleva commented Oct 11, 2023

Close #2626, close #2627, close #2629 and update all workflows/images.

TODO:

  • Use flag.TextVar where possible.
  • Update interop deps.

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Please check atomic copying as well, it seems to be OK except for the cases noted, but maybe there are other cases like that.

pkg/services/rpcsrv/server.go Outdated Show resolved Hide resolved
pkg/services/rpcsrv/server.go Show resolved Hide resolved
pkg/services/rpcsrv/server_test.go Show resolved Hide resolved
pkg/services/rpcsrv/server_test.go Show resolved Hide resolved
pkg/core/stateroot/module.go Show resolved Hide resolved
pkg/network/fuzz_test.go Outdated Show resolved Hide resolved
cli/query/query.go Outdated Show resolved Hide resolved
@AnnaShaleva
Copy link
Member Author

@roman-khimov, I'm still not sure that it will be useful for us integrate flag.TextVar into our repo. Let's take as an example flags.Address and flags.AddressFlag structures and wallet change-password command that uses --address flag:

First of all, we can't completely remove flags.AddressFlag structure because urfave.cli requires a full set of cli.Flag flags to be provided to the command constructor for proper parsing and documentation displaying:

neo-go/cli/wallet/wallet.go

Lines 133 to 145 in b284b90

{
Name: "change-password",
Usage: "change password for accounts",
UsageText: "neo-go wallet change-password -w wallet -a address",
Action: changePassword,
Flags: []cli.Flag{
walletPathFlag,
flags.AddressFlag{
Name: "address, a",
Usage: "address to change password for",
},
},
},

After that, if we're trying to replace flags.Address structure by a single *util.Uint160 value with TextMarshaller interface defined on it, then we can get the following diff:

diff --git a/cli/flags/address.go b/cli/flags/address.go
index f20e9948..52736c10 100644
--- a/cli/flags/address.go
+++ b/cli/flags/address.go
@@ -10,53 +10,20 @@ import (
        "github.com/urfave/cli"
 )
 
-// Address is a wrapper for a Uint160 with flag.Value methods.
-type Address struct {
-       IsSet bool
-       Value util.Uint160
-}
-
 // AddressFlag is a flag with type string.
 type AddressFlag struct {
        Name  string
        Usage string
-       Value Address
+       Value *util.Uint160
 }
 
 var (
-       _ flag.Value = (*Address)(nil)
-       _ cli.Flag   = AddressFlag{}
+       _ cli.Flag = AddressFlag{}
 )
 
-// String implements the fmt.Stringer interface.
-func (a Address) String() string {
-       return address.Uint160ToString(a.Value)
-}
-
-// Set implements the flag.Value interface.
-func (a *Address) Set(s string) error {
-       addr, err := ParseAddress(s)
-       if err != nil {
-               return cli.NewExitError(err, 1)
-       }
-       a.IsSet = true
-       a.Value = addr
-       return nil
-}
-
-// Uint160 casts an address to Uint160.
-func (a *Address) Uint160() (u util.Uint160) {
-       if !a.IsSet {
-               // It is a programmer error to call this method without
-               // checking if the value was provided.
-               panic("address was not set")
-       }
-       return a.Value
-}
-
 // IsSet checks if flag was set to a non-default value.
 func (f AddressFlag) IsSet() bool {
-       return f.Value.IsSet
+       return f.Value != nil
 }
 
 // String returns a readable representation of this value
@@ -86,7 +53,7 @@ func (f AddressFlag) GetName() string {
 // Ignores errors.
 func (f AddressFlag) Apply(set *flag.FlagSet) {
        eachName(f.Name, func(name string) {
-               set.Var(&f.Value, name, f.Usage)
+               set.TextVar(f.Value, name, f.Value, f.Usage)
        })
 }

It's true that such replacement simplifies our flags package code, but at the same time we have to manually perform some utility conversions in the code:

diff --git a/cli/wallet/wallet.go b/cli/wallet/wallet.go
index f5051efb..700746fe 100644
--- a/cli/wallet/wallet.go
+++ b/cli/wallet/wallet.go
@@ -355,10 +355,10 @@ func changePassword(ctx *cli.Context) error {
        if len(wall.Accounts) == 0 {
                return cli.NewExitError("wallet has no accounts", 1)
        }
-       addrFlag := ctx.Generic("address").(*flags.Address)
-       if addrFlag.IsSet {
+       addrFlag := ctx.Generic("address").(*util.Uint160)
+       if addrFlag != nil {
                // Check for account presence first before asking for password.
-               acc := wall.GetAccount(addrFlag.Uint160())
+               acc := wall.GetAccount(*addrFlag)
                if acc == nil {
                        return cli.NewExitError("account is missing", 1)
                }
@@ -370,7 +370,7 @@ func changePassword(ctx *cli.Context) error {
        }
 
        for i := range wall.Accounts {
-               if addrFlag.IsSet && wall.Accounts[i].Address != addrFlag.String() {
+               if addrFlag != nil && wall.Accounts[i].Address != address.Uint160ToString(*addrFlag) {
                        continue
                }
                err := wall.Accounts[i].Decrypt(oldPass, wall.Scrypt)

So my question is: am I got the idea right, is it the code that we'd like to change? The same logic is related to the Fixed8 flag.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
For all components.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Don't need to keep the copy of the official Golang image in our
dockerfile, let's use it as a base builder image.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@roman-khimov
Copy link
Member

@AnnaShaleva, if it doesn't fit well --- forget about it. The idea was to drop these flags, if there are valid reasons why we can't do that then OK, we don't do it. We've got #3097 as well for the future.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #3157 (3c13a3d) into master (b284b90) will increase coverage by 0.08%.
The diff coverage is 76.00%.

❗ Current head 3c13a3d differs from pull request most recent head 645076d. Consider uploading reports for the commit 645076d to get more accurate results

@@            Coverage Diff             @@
##           master    #3157      +/-   ##
==========================================
+ Coverage   85.36%   85.45%   +0.08%     
==========================================
  Files         323      323              
  Lines       43574    43574              
==========================================
+ Hits        37198    37234      +36     
+ Misses       4915     4876      -39     
- Partials     1461     1464       +3     
Files Coverage Δ
pkg/consensus/consensus.go 75.91% <ø> (-0.06%) ⬇️
pkg/core/dao/dao.go 78.17% <100.00%> (ø)
pkg/core/mempool/mem_pool.go 97.14% <ø> (ø)
pkg/network/bqueue/queue.go 82.17% <ø> (-0.18%) ⬇️
pkg/network/server.go 75.37% <ø> (-0.03%) ⬇️
pkg/rpcclient/client.go 86.56% <100.00%> (ø)
pkg/rpcclient/local.go 86.95% <ø> (-0.28%) ⬇️
pkg/rpcclient/wsclient.go 74.95% <ø> (+0.54%) ⬆️
pkg/services/metrics/metrics.go 63.63% <ø> (-0.81%) ⬇️
pkg/services/notary/notary.go 82.99% <ø> (+1.41%) ⬆️
... and 8 more

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

563765b has "Close 2626", should be #2626 at least.

cli/query/query.go Outdated Show resolved Hide resolved
cli/query/query.go Show resolved Hide resolved
cli/query/query.go Outdated Show resolved Hide resolved
Use sync/atomic everywhere and exclude go.uber.org/atomic from go.mod.
Close #2626.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
We often use binary.PutUint*, but almost all these cases have preallocated
buffer of the size that matches exactly the desired one and use a single or
a couple of calls to PutUint*. Thus, I don't think that replacing
binary.PutUint* by AppendUint* will make things better for all these usages.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Close #2629.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@roman-khimov roman-khimov merged commit f31e3f7 into master Oct 12, 2023
12 of 16 checks passed
@roman-khimov roman-khimov deleted the upd-go branch October 12, 2023 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants